thus removing at least part of the method_missing hack, and also,
removing a little of the magic "What is this find_by_stuff and why
can't I see it in the API?"
I hear that it'll speed things up and the stacktraces will be simpler.
I would imagine that User.find_by(conditions_hash) is probably a
common enough usage that it's worth wrapping in a method, even if that
method's implementation is only something like:
def find_by(conditions_hash, *args)
find(:all, :conditions => conditions_hash, *args)
end
Certainly all you're saving is around ten keystrokes, but there's a
clarity benefit, and doesn't require any significant effort to
understand.
The optimisations Ben's patch provides are certainly great from a
performance point of view, but I think that Court3nay's argument still
stand; the one thing that springs to mind when I think about
dynamically-generated methods in Rails is the nightmare of debugging
routes issues - surely we should be trying to remove as much magic as
possible that isn't necessary?
Certainly all you're saving is around ten keystrokes, but there's a
clarity benefit, and doesn't require any significant effort to
understand.
It does, however, add a whole other method to our supported api.
Every feature we add has to be supported in future releases. Saving
10 keystrokes isn't quite enough to get over that hump. There are
some unanswered questions though:
* How would it work with options like :order, :include
* Does it return :first or :all? the name implies :first but your
proposed implementation is :all.
The optimisations Ben's patch provides are certainly great from a
performance point of view, but I think that Court3nay's argument still
stand; the one thing that springs to mind when I think about
dynamically-generated methods in Rails is the nightmare of debugging
routes issues - surely we should be trying to remove as much magic as
possible that isn't necessary?
I hardly think that Site.find_by_domain(request.host) is anywhere near
as magic as some of the edge cases in routes. I think it's pretty
neat, on the other hand:
It does, however, add a whole other method to our supported api.
Every feature we add has to be supported in future releases. Saving
10 keystrokes isn't quite enough to get over that hump.
My point was really that a couple of Rdoc-umentable methods might be
better than a swathe of dynamically-generated ones. It's an
interesting question - is User.find_by_name part of the supported API?
It is... but only when the users table has a name column.
There are some unanswered questions though:
* How would it work with options like :order, :include
* Does it return :first or :all? the name implies :first but your
proposed implementation is :all.
The implementation specifics are certainly important, but distract
from the idea I was trying to put forward, which is to use concrete
methods that can be documented in code than dynamically generating
them, except where there are substantial benefits (such as the
attribute accessors, for example).
I hardly think that Site.find_by_domain(request.host) is anywhere near
as magic as some of the edge cases in routes.
That's fair - it was a relatively cheap shot, designed only to
reinforce the above
> It does, however, add a whole other method to our supported api.
> Every feature we add has to be supported in future releases. Saving
> 10 keystrokes isn't quite enough to get over that hump.
That, or the continuing question "where did this method come from?"
My point was really that a couple of Rdoc-umentable methods might be
better than a swathe of dynamically-generated ones. It's an
interesting question - is User.find_by_name part of the supported API?
It is... but only when the users table has a name column.
> There are some unanswered questions though:
>
> * How would it work with options like :order, :include
> * Does it return :first or :all? the name implies :first but your
> proposed implementation is :all.
The implementation specifics are certainly important, but distract
from the idea I was trying to put forward, which is to use concrete
methods that can be documented in code than dynamically generating
them, except where there are substantial benefits (such as the
attribute accessors, for example).
That's where I was headed. Yay!
> I hardly think that Site.find_by_domain(request.host) is anywhere near
> as magic as some of the edge cases in routes.
Are people really getting confused by the dynamic finders not being in the API? It’s my impression that they are 1) pretty noticeable and identifiable, since they all follow the same pattern, and 2) cool for Rails newbies, so most people try them out and appear to understand how they work (even if they don’t understand how they’re implemented).
I like find_by_domain more (nicer to type and look at) and I also feel that “solving” this is wrong.
You said that the downside of dynamic finders is that they don’t show up in API documentation, and people are confused where the methods got from. Well, this may be true for people who have
never read the basic documentation for AR::Base, because the concept of dynamic finders is nicely outlined there. Those who have quickly realize the methods are a no-brainer. What about dynamic getters and setters on models: how to explain to people that their Post instances have “title” and “published_at” properties? Do those appear in API documentation?
When people started using will_paginate more, I’ve noticed more and more people asked “this paginate_by_domain method, where did it come from? where did ‘paginate’ come from?”. This is because the whole will_paginate API is just method_missing. I’ve then written a lot of Rdoc for the plugin explaining the dynamic nature of the API, and that helped. People understood that will_paginate creates a thin, virtual layer over the existing finder API, part of which is also dynamic (find_by_domain). And they understand find_by_foo_and_bar because they have at least glanced at AR::Base docs.
So in short, I don’t believe the “lack” of documentation and the confusion it “causes” justifies deprecating the API.