On Dec 28, 2011, at 5:55 AM, Jon Leighton wrote: > On 27/12/11 14:32, Matt jones wrote: >> https://github.com/rails/rails/pull/4082 > > I've commented on the PR. >
Updated PR against master: https://github.com/rails/rails/pull/4216 >> On a related note, the current mechanism for handling 'reorder' seems >> pretty strange; what's the design motivation behind keeping the values >> passed to reorder separate from order? > > The reason is so that we can handle eagerly defined scopes correctly. For > example, consider: That makes sense. I was pretty sure there was a good reason. :) > The root problem is that we allow eagerly evaluated scopes to be defined at > all. Personally I dislike 'scope' and would rather that we just told people > to define their own class methods (which of course only get evaluated when > they are actually used). Others like the brevity of the single line 'scope' > syntax. IMO, the real issue with order/reorder here is more about encapsulation - currently, anyone who's digging in the Relation internals needs to know what to do with order_values vs. reorder_value. It doesn't actually happen everywhere - for instance, in find_one: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/finder_methods.rb#L321 the giant conditional skips checking reorder_value. Probably not a major issue (especially since it can only matter if the identity map is on), but still annoying... > For Rails 4 I would like to discuss making use of the new lambda syntax as a > compromise, e.g. > > scope :by_bla, -> { reorder('bla') } Seems sensible to me. I've also encountered a related issue with merging scopes; in short, merging a scope defined with a lambda has to *also* be wrapped in a lambda or else things don't work as intended. For instance: scope :recent, lambda { where('created_at >= ?', 5.days.ago } scope :recent_sorted, merge(recent).order('some_field ASC') winds up prematurely evaluating the recent scope. Not a huge deal when the scopes are defined close to each other, but a recipe for head-scratching when they're farther apart (or in different modules, etc). >> As a side-effect of that >> implementation, this code: >> >> SomeModel.order('foo ASC').reorder('bar DESC').order('baz ASC') >> >> does not (as might be expected) sort the returned record by baz - the >> reorder overrides all previous *and* subsequent orders. > > I think this is a bug. I'll write up a test and a ticket to get that discussion started. Thanks, --Matt Jones -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
