canable icon indicating copy to clipboard operation
canable copied to clipboard

Another canable update - options hash on able methods

Open jopotts opened this issue 13 years ago • 10 comments

Hi again John,

Sorry to fire another pull request to you so soon, but hopefully you'll like this one enough too to include it. Thanks for including the last one btw.

So, this latest update came about from me needing to pass things to the able methods. I've made the changes *mostly backwards compatible with 0.3.0, but had to go down the method missing route on the able class to cope with the potential operator overload. I've updated the readme with an example.

  • The only breaking change is that overloaded methods on the enforcer class will need to have an extra hash parameter.

Have a look and let me know what you think. It definitely adds to the flexibility. No more updates after this for a while I promise.

Cheers, Jo

jopotts avatar Sep 08 '11 12:09 jopotts

Sorry. Last thing (I think). Just added another commit to the pull request - the last commit. It's to be able to change the default result of the able calls.

jopotts avatar Sep 08 '11 15:09 jopotts

I think those are all my thoughts on this. Lets talk more about it if you disagree or feel free to fix them and I'll pull. Thanks for all the work. Making a lot of sense.

jnunemaker avatar Sep 17 '11 01:09 jnunemaker

Hi John,

Thanks for the comments on the last pull request. I agreed with them all and thanks for taking the time to look into it in detail.

I fixed everything you said, with the main thing being the removal of method_missing which is definitely a good thing. I've also made use of the arity check to make it backwards compatible, and it's also nicer like that to be able to have single arg able methods.

Sorry to bundle the addition of the new able_check method and the refactoring into the same commit. Hope you like this one. Comments welcome of course.

Cheers, Jo.

jopotts avatar Sep 19 '11 12:09 jopotts

Hi. Were you waiting for me to do anything else with this? No rush. Just wondering. Cheers.

jopotts avatar Sep 29 '11 15:09 jopotts

Nope. Started on reviewing the pull and then ran out of time. I'll try again this week.

On Thu, Sep 29, 2011 at 11:28 AM, Jo Potts [email protected] wrote:

Hi. Were you waiting for me to do anything else with this? No rush. Just wondering. Cheers.

Reply to this email directly or view it on GitHub: https://github.com/jnunemaker/canable/pull/6#issuecomment-2238925

jnunemaker avatar Oct 01 '11 23:10 jnunemaker

Ok, finally looked this over. I'm confused. I see able_default but can_default seems gone. Now there is an able_check method?

Lets take a step back. What is the goal of this pull request?

  • To be able to pass options to can_...? methods?
  • Ability to set the default able return value?

Anything else? Also, why is the Ables module changed? What was wrong with the way it was doing things before. I think this is part of of what is confusing me.

jnunemaker avatar Oct 06 '11 14:10 jnunemaker

Sometimes it takes me longer to understand others code, so please be patient! :)

jnunemaker avatar Oct 06 '11 14:10 jnunemaker

No worries about the questions. Best to get it right.

You're right about the goals: Sending options (to class and instance methods of able_by? methods), and setting a default.

The able_check and default_able class methods were added simply for flexibility's sake (to make the gem more attractive).

The can_default change to able_default was simply a name change that seemed like a good idea at the time. Perhaps can_default is better? Sorry for the confusion on that one.

I think you get it already, but have a look at the explanations added to the readme under the titles; Passing Additional Arguments to Ables, and Changing the Default Able. They give some simple examples.

Code-wise, the reason the adding of the #{able}_by methods has been moved to where it is, is because I'm using class_eval to add class methods which wouldn't be possible to do using module_eval like before.

Hope that helps! :)

jopotts avatar Oct 06 '11 15:10 jopotts

Just a quick reminder in case I catch you with a moment to spare! I'm using this on a couple of sites, so it would be great to get it pulled to your gem at some point. No pressure.

jopotts avatar Dec 06 '11 12:12 jopotts

Hi John. Have you had a chance to look at this again yet?

jopotts avatar May 31 '12 09:05 jopotts