Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #4456 (closed defect: fixed)

Opened 2 years ago

Last modified 2 years ago

[PATCH] add_limit! third argument should have default value, not be required

Reported by: reklam@nyh.se Assigned to: David
Priority: normal Milestone:
Component: ActiveRecord Version: 1.1.0 RC1
Severity: normal Keywords:
Cc:

Description

Got some strange results when upgrading to Rails 1.1 from 1.0, traced it to its source and found that ActiveRecord::Base::add_limit! now requires three arguments, from only two in 1.0. I would think the third, "scope", argument should not be required and therefore should be defined like 

def add_limit!(sql, options, scope = nil)
...

or something in that vein, rather than 

def add_limit!(sql, options, scope)
...

as the case is now. I didn't see this third argument mentioned in the documentation, either.

Change History

03/28/06 14:28:03 changed by rick

  • status changed from new to closed.
  • resolution set to invalid.

Just pass the :find scope to it: 

add_limit!(sql, options, scope(:find))

The private ActiveRecord find methods got an overhaul in 1.1 for optimization. The previous version was making multiple unnecessary calls to find the current :find scope.

03/28/06 14:33:59 changed by henrik@nyh.se

  • status changed from closed to reopened.
  • resolution deleted.

Thank you for the clarification. Might I then suggest adding scope(:find) as the default argument value, assuming scope(:find) doesn't have to be passed in from outside? If this works, it would mean backwards compatibility as well as cleaner code.

Re-opening the ticket because of this suggestion, not sure if that's proper ticket handling or not...

03/28/06 14:49:10 changed by anonymous

Adding scope(:find) as default will make construct_finder_sql slower again. And add_limit! isn't the only method which would require defaults for scope: There are methods add_conditions!, and add_joins! as well.

Additionally, I found that it's very easy to simply forget the last parameter when writing code using add_limit!. And if additional scopes were defined, e.g. an update scope, then the find default would not make sense at all.

03/28/06 14:52:52 changed by anonymous

OK. I don't have a good grasp of what scope() does in this case, and Google wasn't very helpful. If anyone could point me to a good web page or book that explains it, I'd be very grateful.

04/01/06 23:15:38 changed by rick

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [4129]) Private ActiveRecord methods add_limit, add_joins, and add_conditions take an OPTIONAL third argument 'scope' (closes #4456) [Rick]

04/03/06 12:12:06 changed by skaes

  • status changed from closed to reopened.
  • resolution deleted.

Changeset [4129] breaks performance for find. I wish it could be rolled back. These are private methods. If we can't change private methods, we're doomed.

04/03/06 16:09:49 changed by rick

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [4141]) Changed those private ActiveRecord methods to take optional third argument :auto instead of nil for performance optimizations. (closes #4456) [Stefan]

08/06/06 10:01:42 changed by rick

  • reporter changed from henrik@nyh.se to reklam@nyh.se.