Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

cannot semantics for class changed from 1.3.3 to 1.3.4 #161

Closed
coshx opened this Issue · 5 comments

2 participants

@coshx

This changed in commit 5a353c1

@ability.can :read, :all
@ability.cannot :read, Integer
@ability.can?(:read, Integer)

I would expect the above to evaluate to false, but the cannot definition is no longer considered a match. I wonder if definition matching needs to be made more powerful, by matching with the most specific definition? Take for example the following:

@ability.cannot :read, Fixnum
@ability.can :read, Integer
@ability.can?(:read, 123)

Should that evaluate to true, because the most recent rule takes precedence, or should it evaluate to false, because the most specific rule takes precedence?

@ryanb
Owner

Thanks for pointing that out. That commit was specifically targeting cases where a hash of conditions are used, so perhaps if I revert to the old behavior when conditions are not used it will satisfy both cases.

Regarding your last example, I believe the old behavior would take the most recent matching can/cannot definition, so that can? would return true.

@coshx

It seems inconsistent to take the mot recent match in one case, but the most specific match in another case (where both a class and a hash of conditions is used). I wonder if you could keep things simple by saying it's always the most recent match, and then just ensuring that the definition is only a match if the hash also matches. So in the following example, the cannot rule should only match in the second test:

@ability.can :read, Range
@ability.cannot :read, Range, :begin => 1

@ability.can?(:read, 2..5).should be_true
@ability.can?(:read, 1..5).should be_false
@ryanb
Owner

When a class is passed in it needs different behavior:

@ability.can :read, Range
@ability.cannot :read, Range, :begin => 1
@ability.can?(:read, Range).should be_true

Therefore it cannot always take the first matching case because a hash condition is considered matching.

In complex situations like this, I prefer to fix one test case at a time and let the implementation evolve on its own through. The mentioned solution should work in this case, if there's another practical situation where it does not behave as expected we can tackle that when it comes up.

@ryanb
Owner

don't stop at cannot definitions when there are no conditions - closed by 8f49f28

@coshx

Thanks! :)

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.