Permalink
Browse files

Deprecate defining scopes with a callable (lambda, proc, etc) via the…

… scope class method. Just define a class method yourself instead.
  • Loading branch information...
1 parent 788bd30 commit f0e198bfa1e3f9689e0cde1d194a44027fc90b3c @jonleighton jonleighton committed with tenderlove Apr 8, 2011
View
18 activerecord/CHANGELOG
@@ -1,5 +1,23 @@
*Rails 3.1.0 (unreleased)*
+* Passing a proc (or other object that responds to #call) to scope is deprecated. If you need your
+ scope to be lazily evaluated, or takes parameters, please define it as a normal class method
+ instead. For example, change this:
+
+ class Post < ActiveRecord::Base
+ scope :unpublished, lambda { where('published_at > ?', Time.now) }
+ end
+
+ To this:
+
+ class Post < ActiveRecord::Base
+ def self.unpublished
+ where('published_at > ?', Time.now)
+ end
+ end
+
+ [Jon Leighton]
+
* Default scopes are now evaluated at the latest possible moment, to avoid problems where
scopes would be created which would implicitly contain the default scope, which would then
be impossible to get rid of via Model.unscoped.
View
62 activerecord/lib/active_record/named_scope.rb
@@ -51,6 +51,14 @@ def scoped(options = nil)
# The above calls to <tt>scope</tt> define class methods Shirt.red and Shirt.dry_clean_only. Shirt.red,
# in effect, represents the query <tt>Shirt.where(:color => 'red')</tt>.
#
+ # Note that this is simply 'syntactic sugar' for defining an actual class method:
+ #
+ # class Shirt < ActiveRecord::Base
+ # def self.red
+ # where(:color => 'red')
+ # end
+ # end
+ #
# Unlike <tt>Shirt.find(...)</tt>, however, the object returned by Shirt.red is not an Array; it
# resembles the association object constructed by a <tt>has_many</tt> declaration. For instance,
# you can invoke <tt>Shirt.red.first</tt>, <tt>Shirt.red.count</tt>, <tt>Shirt.red.where(:size => 'small')</tt>.
@@ -74,14 +82,34 @@ def scoped(options = nil)
# then <tt>elton.shirts.red.dry_clean_only</tt> will return all of Elton's red, dry clean
# only shirts.
#
- # Named \scopes can also be procedural:
+ # If you need to pass parameters to a scope, define it as a normal method:
#
# class Shirt < ActiveRecord::Base
- # scope :colored, lambda {|color| where(:color => color) }
+ # def self.colored(color)
+ # where(:color => color)
+ # end
# end
#
# In this example, <tt>Shirt.colored('puce')</tt> finds all puce shirts.
#
+ # Note that scopes defined with \scope will be evaluated when they are defined, rather than
+ # when they are used. For example, the following would be incorrect:
+ #
+ # class Post < ActiveRecord::Base
+ # scope :recent, where('published_at >= ?', Time.now - 1.week)
+ # end
+ #
+ # The example above would be 'frozen' to the <tt>Time.now</tt> value when the <tt>Post</tt>
+ # class was defined, and so the resultant SQL query would always be the same. The correct
+ # way to do this would be via a class method, which will re-evaluate the scope each time
+ # it is called:
+ #
+ # class Post < ActiveRecord::Base
+ # def self.recent
+ # where('published_at >= ?', Time.now - 1.week)
+ # end
+ # end
+ #
# Named \scopes can also have extensions, just as with <tt>has_many</tt> declarations:
#
# class Shirt < ActiveRecord::Base
@@ -92,6 +120,18 @@ def scoped(options = nil)
# end
# end
#
+ # The above could also be written as a class method like so:
+ #
+ # class Shirt < ActiveRecord::Base
+ # def self.red
+ # where(:color => 'red').extending do
+ # def dom_id
+ # 'red_shirts'
+ # end
+ # end
+ # end
+ # end
+ #
# Scopes can also be used while creating/building a record.
#
# class Article < ActiveRecord::Base
@@ -128,6 +168,24 @@ def scope(name, scope_options = {})
valid_scope_name?(name)
extension = Module.new(&Proc.new) if block_given?
+ if !scope_options.is_a?(Relation) && scope_options.respond_to?(:call)
+ ActiveSupport::Deprecation.warn <<-WARN
+Passing a proc (or other object that responds to #call) to scope is deprecated. If you need your scope to be lazily evaluated, or takes parameters, please define it as a normal class method instead. For example, change this:
+
+class Post < ActiveRecord::Base
+ scope :unpublished, lambda { where('published_at > ?', Time.now) }
+end
+
+To this:
+
+class Post < ActiveRecord::Base
+ def self.unpublished
+ where('published_at > ?', Time.now)
+ end
+end
+ WARN
+ end
+
scope_proc = lambda do |*args|
options = scope_options.respond_to?(:call) ? scope_options.call(*args) : scope_options
options = scoped.apply_finder_options(options) if options.is_a?(Hash)
View
6 activerecord/test/cases/named_scope_test.rb
@@ -471,6 +471,12 @@ def test_scoped_are_lazy_loaded_if_table_still_does_not_exist
require "models/without_table"
end
end
+
+ def test_scopes_with_callables_are_deprecated
+ assert_deprecated do
+ Post.scope :WE_SO_EXCITED, lambda { |partyingpartyingpartying, yeah| fun!.fun!.fun! }
+ end
+ end
end
class DynamicScopeMatchTest < ActiveRecord::TestCase
View
5 activerecord/test/models/comment.rb
@@ -1,5 +1,8 @@
class Comment < ActiveRecord::Base
- scope :limit_by, lambda {|l| limit(l) }
+ def self.limit_by(l)
+ limit(l)
+ end
+
scope :containing_the_letter_e, :conditions => "comments.body LIKE '%e%'"
scope :not_again, where("comments.body NOT LIKE '%again%'")
scope :for_first_post, :conditions => { :post_id => 1 }
View
22 activerecord/test/models/post.rb
@@ -7,12 +7,15 @@ def author
scope :containing_the_letter_a, where("body LIKE '%a%'")
scope :ranked_by_comments, order("comments_count DESC")
- scope :limit_by, lambda {|l| limit(l) }
- scope :with_authors_at_address, lambda { |address| {
- :conditions => [ 'authors.author_address_id = ?', address.id ],
- :joins => 'JOIN authors ON authors.id = posts.author_id'
- }
- }
+
+ def self.limit_by(l)
+ limit(l)
+ end
+
+ def self.with_authors_at_address(address)
+ where('authors.author_address_id = ?', address.id)
+ .joins('JOIN authors ON authors.id = posts.author_id')
+ end
belongs_to :author do
def greeting
@@ -27,9 +30,10 @@ def greeting
scope :with_special_comments, :joins => :comments, :conditions => {:comments => {:type => 'SpecialComment'} }
scope :with_very_special_comments, joins(:comments).where(:comments => {:type => 'VerySpecialComment'})
- scope :with_post, lambda {|post_id|
- { :joins => :comments, :conditions => {:comments => {:post_id => post_id} } }
- }
+
+ def self.with_post(post_id)
+ joins(:comments).where(:comments => { :post_id => post_id })
+ end
has_many :comments do
def find_most_recent
View
26 activerecord/test/models/topic.rb
@@ -1,10 +1,20 @@
class Topic < ActiveRecord::Base
scope :base
- scope :written_before, lambda { |time|
- if time
- { :conditions => ['written_on < ?', time] }
- end
- }
+
+ ActiveSupport::Deprecation.silence do
+ scope :written_before, lambda { |time|
+ if time
+ { :conditions => ['written_on < ?', time] }
+ end
+ }
+
+ scope :with_object, Class.new(Struct.new(:klass)) {
+ def call
+ klass.where(:approved => true)
+ end
+ }.new(self)
+ end
+
scope :approved, :conditions => {:approved => true}
scope :rejected, :conditions => {:approved => false}
@@ -19,12 +29,6 @@ def one
end
end
- scope :with_object, Class.new(Struct.new(:klass)) {
- def call
- klass.where(:approved => true)
- end
- }.new(self)
-
module NamedExtension
def two
2

13 comments on commit f0e198b

@janx

wow, nice!

@parndt

So what does this mean for default_scope, now can't we have default_scope use a callable?

@fesplugas

I've been avoiding the use of lambdas when definining scopes and using class methods instead. This definitely will make things easier.

@ook

From my POV, it'll be a little bit less easy to determine what scopes are available for a Model, which is bad. :-/

@dmathieu

I completely agree with @ook. It makes scopes less easy to find (and almost useless).
@parndt's question also triggers my curiosity.

@jcasimir

Because scopes were unnecessary already, dmathieu. I'm happy we're going this direction!

@jonleighton
Ruby on Rails member

@parndt: You can, just call your callable within the default_scope class method.

@ook: In what situation do you need to "determine what scopes are available for a model"?

@ook

@jonleighton: Code maintainability: I changed of firm last year, on a first project, there wasn't any scope at all: terribly slow to "take" the model. Then a second project with scopes: I was operationnal to work on that project in 2hours. I'm pretty sure we'll see a new convention in rdoc to mark model class methods which return a scope.

@jonleighton
Ruby on Rails member

Ok, well the reasons for the recent scopes changes were:

  1. Simplify the internal Rails code
  2. Fix problem with default_scopes being evaluated too soon and becoming "embedded" in other scopes
  3. The "scope :foo, lambda { ... }" change was made because it is basically a very ugly way of defining what is essentially a method.

You can still define scopes like "scope :foo, where(:bla)" if you wish.

@jonleighton
Ruby on Rails member

This has been reverted now actually: 256b363

@dhh was keen to have consistency between the syntax for regular scopes and scopes that you need to be evaluated lazily.

We're being clearer about it being syntactic sugar in the docs now though: https://github.com/rails/rails/blob/28146378d3c83ac8c0ea3427b6152ea61976d642/activerecord/lib/active_record/named_scope.rb#L54

Also, the 1.9 'stabby lambda' makes this syntax a bit easier on the eyes, e.g.

scope :recent, -> { where(:posted_at => Time.now - 1.week) }
@exviva

Couldn't the scope take a &block as the last parameter? Then these examples would be something like:
scope :recent { where(:posted_at => Time.now - 1.week) }

@jonleighton
Ruby on Rails member

@exviva: scope already takes a block, which defines an extension module in the same way as associations. Besides, your example is syntactically invalid because in order to have a block param on a single line line that you need to use parentheses around the args (e.g. scope(:recent) { .. }).

@exviva

@jonleighton, ah, I forgot about extension modules. And of course you're right, there's a syntax error.

But the question now is: what's a more common use case - lazy scopes or extension modules? I'd say (personal opinion) lazy scopes, so maybe they should get the nicer syntax. Extension module syntax is nasty anyway, and probably it'd make sense to promote other solutions (perhaps scope :foo, where(:this => 'that'), :extension => MyExtension).

Please sign in to comment.