Permalink
Browse files

fix a scope not using a lambda but it should have been

scope without lambda's are bad
  • Loading branch information...
1 parent 0431a2c commit ac10c372e7bdec356b0745181acb7b623ab8f262 Jean-Philippe Boily committed Oct 18, 2012
Showing with 1 addition and 1 deletion.
  1. +1 −1 app/models/monologue/post.rb
@@ -8,7 +8,7 @@ class Monologue::Post < ActiveRecord::Base
attr_accessible :posts_revisions_attributes, :published, :tag_list
scope :default, includes(:posts_revisions).where("posts_revision_id = monologue_posts_revisions.id").order("published_at DESC, monologue_posts.created_at DESC, monologue_posts.updated_at DESC")
- scope :published, default.where(:published => true).where("published_at <= ?", DateTime.now)
+ scope :published, lambda { default.where(:published => true).where("published_at <= ?", DateTime.now) }
default_scope includes(:tags)

6 comments on commit ac10c37

Collaborator

msevestre replied Oct 18, 2012

Why is that?

Collaborator

msevestre replied Oct 18, 2012

As far as i remember rails best practices this is okay

Owner

jipiboily replied Oct 18, 2012

Scopes are cached if not in a lambda, including the DateTime value. Anyway eager evaluated scopes will be deprecated in Rails 4.0...

See here and search for "Deprecate eager-evaluated scopes".

There is certainly better source for that, but here's one: http://stackoverflow.com/questions/11525169/scope-evaluate-time-in-rails-3

Collaborator

msevestre replied Oct 18, 2012

interesting. You might want to change default_scope default_scope { ... } then
What about the "default" scope line 10?

Owner

jipiboily replied Oct 18, 2012

well, the "default" scope on line should have another name. It's the default for the font facing app, not in the admin...

As for the default_scope, it's not a problem as there is no problem for it to be cached. We could move all scope to lambda's that said. I want to work on supporting Rails 4 "soon".

Collaborator

msevestre replied Oct 18, 2012

It was just to comment on the scope without lambda's are bad :-)

Please sign in to comment.