Override the find method #376

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
3 participants

Why not go a step further?

finder =
  if @options[:find_by]
    "find_by_#{@options[:find_by]}!"
  elsif @options[:finder]
    @options[:finder]
  else
    current_ability.model_adapter.finder
  end
resource_base.send(finder, id_param)

Much more readable :)
(it is equivalent right?)

So what do you think of maxsum-corin@b013066?

I don't see the need for yet another option, except for when there's a conflict in names...

Corin Lawson Ooops! 64e9f47

Sorry, living on the edge here... I'll do some quick tests.

Corin Lawson added some commits May 20, 2011

Corin Lawson Fix syntax 78a1ba3
Corin Lawson Fix syntax 832b71a

Um, I get wrong number of arguments (0 for 2) on this line:

    current_ability.model_adapter.finder

help?

Oh, we need some args for model_adapter :/

should adapter_class be cached?

current_ability.model_adapter(resource_base, @params[:action]).finder right? Will @params[:action] be set already?

Corin Lawson Fix syntax 86bbc96
Owner

ryanb commented May 20, 2011

Thanks for this, however I don't see any tests for this. I am also thinking about making the :find_by option dual purpose so we don't have to introduce another :finder option. It can see if respond_to? @options[:find_by] and fall back to find_by_#{@options[:find_by]} or maybe visa-versa. See issue #335 for more info.

Using the model adapter for the default find method is a good idea. I was planning to do this in 2.0 but should probably backport to 1.6 as well. I don't care for the adapter returning a string, it can perform the actual find call given a model class. The reason being is the adapter will need to take on other behavior as well when building records, etc. It's not always a simple method call.

Oh hey I don't see #335... This is pretty much what I did in maxsum-corin@b013066! Could you elaborate on your find_using_slug idea?

Also can give some quick tips or guidance for the tests?

Regarding the model_adapter#finder method; it seems appropriate to me that it returns the name of the finder method since that's how :find_by option works.

If you want to go down that track :find_by really ought to accept a Proc too

          if ...
            resource_base.send("find_by_#{@options[:find_by]}!", id_param)
          elsif ...
            resource_base.send(@options[:find_by], id_param)
          elsif ...
            @options[:find_by].call(resource_base, id_param)
          elsif ...
            resource_base.send(@options[:finder], id_param)
          elsif ...
            @options[:finder].call(resource_base, id_param)
          else
            current_ability.model_adapter(resource_base, @params[:action]).find(id_param)
          end
Owner

ryanb commented May 21, 2011

Closing because I added some commits for this functionality. I don't want to add a proc to find_by because complex find behavior should go in the model. The default find behavior may need to be complex which is why the adapter handle's the behavior and doesn't simply return a method name.

ryanb closed this May 21, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment