Rails Ruby on Rails → 3.1

Comments and changes to this ticket

• You flagged this item as spam.
  

  2kan January 15th, 2011 @ 12:52 AM

  Here is my patch (with tests) for it.

  • 6290-fix-against-rails-edge.diff 4.4 KB

• You flagged this item as spam.
  

  2kan January 15th, 2011 @ 12:55 AM

  • Assigned user changed from “Santiago Pastorino” to “Aaron Patterson”
  • Tag changed from “activerecord scopes, rails edge, scope” to “activerecord scopes, rails edge, bug, patch, scope”
• 

  Santiago Pastorino February 2nd, 2011 @ 06:30 PM

  • State changed from “new” to “open”
  • Milestone set to “3.1”
  • Importance changed from “” to “Low”

  The expression scope :reorder_by_name, except(:order).order("name DESC") is computed at evaluation time.

  You need to do this ...

  class A < ActiveRecord::Base
    scope :order_by_salary, order("salary DESC")
    scope :reorder_by_name, lambda { except(:order).order("name DESC") }
  end
  

  Anyways seems the code I showed you is not currently working on master with named scopes even though it works if I do ...

  >> Post.order(:title).to_sql
  => "SELECT \"posts\".* FROM \"posts\"  ORDER BY title"
  >> Post.order(:title).except(:order).to_sql
  => "SELECT \"posts\".* FROM \"posts\" "
  >> Post.order(:title).except(:order).order(:body).to_sql
  => "SELECT \"posts\".* FROM \"posts\"  ORDER BY body"
  

  So we need to fix the issue for named scopes.

• You flagged this item as spam.
  

  Adam Wróbel February 2nd, 2011 @ 11:27 PM

  I suggest moving order from MULTI_VALUE_METHODS to SINGLE_VALUE_METHODS and overwriting the order array with each merged scope or new call to order(). There's no benefit in using:
  

  Employee.order(:division_name).order(:name)
  

  over
  

  Employee.order(:division_name,:name)
  

  And the fact that both of the below calls return Posts by [:created_at,:title] order is just plain misleading:
  

  class Post < ActiveRecord::Base
    default_scope order(:created_at)
    scope :by_title, order(:title)
  end
  Post.order(:title)
  Post.by_title
  

• You flagged this item as spam.
  

  2kan February 2nd, 2011 @ 11:50 PM

  Adam Wróbel, order is a multi value and I can give you a lot of examples from my real apps where you need to use queries like:

  SELECT * from employers WHERE ORDER BY salary ASC, name ASC
  

  Here you sort employers by salary and if salary is equal you sort then by name. It is a common task. And so if we're allow user to write order('salary ASC, name ASC') I think we should allow to write order(:salary).order(:name).

• You flagged this item as spam.
  

  2kan February 2nd, 2011 @ 11:56 PM

  Adam Wróbel, i mean that it is not intuitive why we can do:

  order(:division_name).order(:name)
  

  and can't:

  Employee.order(:division_name,:name)
  

  or do something like:

  class Post < ActiveRecord::Base
   scope popular, where('rating > 1').order('rating DESC')
   scope recent, order('created_at DESC')
  end
  
  Post.popular.recent
  

• You flagged this item as spam.
  

  2kan February 2nd, 2011 @ 11:58 PM

  Oh, sorry, it is 2:57 AM here. In my previous comment we can't do first and can second and so can't do the last example with a Post class.

• You flagged this item as spam.
  

  Adam Wróbel February 3rd, 2011 @ 12:05 AM

  An alternative would be to overwrite on merge, but append on order(). That would be similar to where(:column => value). Multiple where() calls create OR but during a merge only the last one is left.

• You flagged this item as spam.
  

  Adam Wróbel February 3rd, 2011 @ 12:22 AM

  I know you specify multiple columns for order, but when I referred to multi and single values I meant values that get merged and values that get overwritten. My suggestion was to overwrite array of order columns from the current scope with array of order columns from the relation being merged in.

  The Post.popular.recent example makes sense and might be a reason for why none of my suggestions should be used, but then again order is such a simple operation that it's easy to list all the columns you need in one place. Usually I'd just be confused that my last call to order wasn't the one used rather than happy that I was was able to merge two named order scopes. Take this test from rails tests (tests/cases/relations_test.rb):

  def test_order_using_scoping
    car1 = CoolCar.order('id DESC').scoping do
      CoolCar.find(:first, :order => 'id asc')
    end
    assert_equal 'zyke', car1.name
  
    car2 = FastCar.order('id DESC').scoping do
      FastCar.find(:first, :order => 'id asc')
    end
    assert_equal 'zyke', car2.name
  end
  

  To me it reads "def test_we_have_a_bug". I mean - I've specified the order I want right there - in the call to find() - why do you do it reverse, rails?

• You flagged this item as spam.
  

  2kan February 3rd, 2011 @ 04:34 PM

  Santiago Pastorino, Yes this code computes at evaluation time, but it doesn't matter. I think that there is no reason (point me if i'm wrong) to allow user to use except only inside lambdas. I understand that I need to use lambda when I use Time.now or something but here I need some additional knowledge about how except, order, where and etc works inside rails. I don't think that it is good. I've added to my patch test with scope with lambda like:

  lambda { except(:order).order("name DESC") }
  

  and test passes.

  How it works and why I think we can calculate except at evaluation time. A've added a flag into the Relation that it is except and store what we want to except, so when we merge relations when calling the scopes we can figure out that we need to except some values.

  Please, point me if I made a mistake somewhere.

  • 6290-fix-against-rails-edge.diff 4.4 KB

• You flagged this item as spam.
  

  2kan March 21st, 2011 @ 01:44 PM

  Oops, I've found that my last uploaded patch wasn't good enough (I've uploaded the wrong diff, sorry). Here is the correct one.

  • 6290-fix-against-rails-edge.diff 4.3 KB