Another canable update - options hash on able methods #6

Open
wants to merge 10 commits into
from

Conversation

Projects
None yet
2 participants
@jopotts
Contributor

jopotts commented Sep 8, 2011

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

This comment has been minimized.

Show comment Hide comment
@jopotts

jopotts Sep 8, 2011

Contributor

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.

Contributor

jopotts commented Sep 8, 2011

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.

README.rdoc
+
+You can easily change the default on the able methods from true to false to lock down security more. Simply add the following to your canable initializer file (./config/initializers/canable.rb):
+
+ Canable.default_canability = false

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Sep 17, 2011

Owner

Thinking this seems kind of long. Wondering if Canable.can_default or something would be enough?

@jnunemaker

jnunemaker Sep 17, 2011

Owner

Thinking this seems kind of long. Wondering if Canable.can_default or something would be enough?

README.rdoc
+ include Canable::Ables
+
+ # Only allow viewable by default
+ def self.default_canability(able_name)

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Sep 17, 2011

Owner

Same thought here as the comment above.

@jnunemaker

jnunemaker Sep 17, 2011

Owner

Same thought here as the comment above.

@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Sep 17, 2011

Instead of method missing, can we just check the method arity before calling? Or just make a backwards compat break?

Instead of method missing, can we just check the method arity before calling? Or just make a backwards compat break?

@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Sep 17, 2011

Instead of @Klass = klass, just change klass to @Klass in line 46.

Instead of @Klass = klass, just change klass to @Klass in line 46.

@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Sep 17, 2011

I think this should be default to true.

I think this should be default to true.

@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Sep 17, 2011

This should be true too.

This should be true too.

@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Sep 17, 2011

Owner

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.

Owner

jnunemaker commented Sep 17, 2011

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.

@jopotts

This comment has been minimized.

Show comment Hide comment
@jopotts

jopotts Sep 19, 2011

Owner

I'm not sure why it needs this. Fails test without.

I'm not sure why it needs this. Fails test without.

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Sep 21, 2011

Off the top of my head: probably need to ensure that able and can defaults are set to correct default in the setup of all tests.

Off the top of my head: probably need to ensure that able and can defaults are set to correct default in the setup of all tests.

@jopotts

This comment has been minimized.

Show comment Hide comment
@jopotts

jopotts Sep 19, 2011

Contributor

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.

Contributor

jopotts commented Sep 19, 2011

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

This comment has been minimized.

Show comment Hide comment
@jopotts

jopotts Sep 19, 2011

Owner

I had to update this class to use a real (as opposed to mocked) resource due to the check on the arity of the able methods.

I had to update this class to use a real (as opposed to mocked) resource due to the check on the arity of the able methods.

@jopotts

This comment has been minimized.

Show comment Hide comment
@jopotts

jopotts Sep 19, 2011

Owner

Now backwards compatible.

Now backwards compatible.

@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Sep 21, 2011

Personally I believe in forcing an API rather than supporting all kinds of arguments. I might tweak this after pulling. We'll see.

Personally I believe in forcing an API rather than supporting all kinds of arguments. I might tweak this after pulling. We'll see.

@jopotts

This comment has been minimized.

Show comment Hide comment
@jopotts

jopotts Sep 29, 2011

Contributor

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

Contributor

jopotts commented Sep 29, 2011

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

@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Oct 1, 2011

Owner

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
reply@reply.github.com
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:
#6 (comment)

Owner

jnunemaker commented Oct 1, 2011

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
reply@reply.github.com
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:
#6 (comment)

@jnunemaker

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Oct 6, 2011

Owner

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.

Owner

jnunemaker commented Oct 6, 2011

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

This comment has been minimized.

Show comment Hide comment
@jnunemaker

jnunemaker Oct 6, 2011

Owner

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

Owner

jnunemaker commented Oct 6, 2011

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

@jopotts

This comment has been minimized.

Show comment Hide comment
@jopotts

jopotts Oct 6, 2011

Contributor

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! :)

Contributor

jopotts commented Oct 6, 2011

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

This comment has been minimized.

Show comment Hide comment
@jopotts

jopotts Dec 6, 2011

Contributor

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.

Contributor

jopotts commented Dec 6, 2011

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.

Pass instance through to able_check.
When an able check on an instance of an object falls through to the catch-all able_check class method, pass through the object's instance in the option hash.
@jopotts

This comment has been minimized.

Show comment Hide comment
@jopotts

jopotts May 31, 2012

Contributor

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

Contributor

jopotts commented May 31, 2012

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

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