Make the scopes play nice with other scopes...

[james2m]

Looking around the code there are a lot of hangovers from the Rails2.3 days where the scope chain ends with .find(:all, options). This effectively stops any scope being chained on the end as find and all return Arrays as opposed to ActiveRecord::Relation objects. e.g.;

      # Returns the follow records related to this instance with the followable included.
      def all_follows(options={})
        self.follows.unblocked.includes(:followable).all(options)
      end

Since scopes are far more flexible than an options hash are there any objections to breaking compatibility and removing the all / finds that are on the end of some scope chains?

[tcocca]

I have often thought about this but i have no idea how we could make it work for some of the methods, for example in the followable lib:

def followers(options={})
  self.followings.unblocked.includes(:follower).all(options).collect{|f| f.follower}
end

We don't want to return the "followings" from this method, we want the actual followers, which means you need to do the collection. Any ideas there, otherwise I agree, we could drop all of the .find(:all, options) or .all(options) and no more taking an options hash for the methods, and just return the reference to the scope or arel chain or whatever you want to call it and let people continue to chain onto it.

On the other hand, what it we provide I deprecation period for those methods. Leave the options hash in place and only append the .all(options) if that hash is not empty, and issue a deperecation in those methods.

Thoughts?

My main concern is the methods that return the referenced follower or followable objects, not the actual "follow" instance, because how do we get at them? I would actually almost say we just leave those methods the way they are.

~ Tom

[james2m]

Here's a thought on the deprecation. If there is an option hash we return a deprecation warning and append the all(options) or find(:all, options) otherwise we return the ActiveRecord::Relation so it becomes useable straight away.

We should set a release version where the options hash is deprecated. 0.2.0?

The major issue to the example above is the double polymorphic join obviously doesn't allow :through because it doesn't know what tables is it is joining to. There is now way to make that example return an ActiveRecord::Relation because the Relation is just a construct that asks Arel to build an SQL query for it so there is no way to turn an Array into a Relation.

We could clean up most methods to return a Relation (this may fix the ticket about SearchLogic).

If you are happy with this I'll keep churning through adding to that little pile of commits to simplify things. Then I'll do a round of deprecating the options hash as a separate exercise. Many small commits rather than one big change?

[tcocca]

Thats exactly what i was thinking - if the options hash arg isn't blank return the all(options) with a deprecation, otherwise return the relation.

Agreed, some methods due to the double polymorphism will still need to do the collection and they should continue to take the options hash in my opinion

If people want to chain something on to that #followers method for example its not that hard to roll their own ...

self.followings.unblocked.includes(:follower).limit(10).order(:follower_type) .... idk, its basically just a convenience method.

I think keeping 1 pull request for simplification, and then a separate pull request for deprecation is the best way to do.

The other thing I am completely on the fence about is the generated model, some people want it to stay in the plugin/gem others want the generated model. I personally like the generated model, its easier to extend rather than monkey patching the thing i guess. Thoughts?

~ Tom

[james2m]

I agree with your generated model. My version of it I re-wrote all the scopes into class methods (as they more consistent in behaviour that scopes with lambdas) so I just have;

  extend ActsAsFollower::FollowerLib
  extend ActsAsFollower::FollowScopes

  # NOTE: Follows belong to the "followable" interface, and also to followers
  belongs_to :followable, :polymorphic => true
  belongs_to :follower,   :polymorphic => true

If it was also an Engine then those who don't want to override the defaults could have that and the generator then creates a model that overrides it so you please all parties. I think that's how Devise does it.