New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for proper attribute-setting in AllowValueMatcher #273

Merged
merged 2 commits into from Mar 28, 2013

Conversation

Projects
None yet
3 participants
@mxie
Contributor

mxie commented Mar 27, 2013

To fix #179:
There was an issue where if an attribute is of integer type and we set it using a string (like when we test for disallowing of ARBITRARY_OUTSIDE_STRING), the attribute will actually be set to 0 instead. Consequently, this causes issues for when we actually want to allow 0.

Show outdated Hide outdated NEWS.md Outdated
Show outdated Hide outdated NEWS.md Outdated
@value = value
@instance.send("#{@attribute}=", @value)
errors_match?
self.instance = instance

This comment has been minimized.

@drapergeek

drapergeek Mar 28, 2013

Contributor

White space after this line.

@drapergeek

drapergeek Mar 28, 2013

Contributor

White space after this line.

Fixes #179: Check that attributes are set properly when testing for m…
…atches

* AllowValueMatcher's `matches?` should check that the attribute was actually set to the value passed to it
@drapergeek

This comment has been minimized.

Show comment
Hide comment
@drapergeek

drapergeek Mar 28, 2013

Contributor

👍

Contributor

drapergeek commented Mar 28, 2013

👍

@mxie mxie merged commit 5da9f52 into master Mar 28, 2013

@mxie mxie deleted the mx-fix-inclusion-for-zero branch Mar 28, 2013

@gabebw

This comment has been minimized.

Show comment
Hide comment
@gabebw

gabebw Mar 28, 2013

Contributor

What's the thinking behind this? Why not use instance variables? I believe self.thing = "foo" is the exact same as @thing = "foo" when there's an accessor.

Contributor

gabebw commented on 5da9f52 Mar 28, 2013

What's the thinking behind this? Why not use instance variables? I believe self.thing = "foo" is the exact same as @thing = "foo" when there's an accessor.

This comment has been minimized.

Show comment
Hide comment
@mxie

mxie Mar 28, 2013

Contributor

I think it makes sense for us to remove uses of instance variables and use barewords instead to make their implementations transparent. However, I do think that for assignments, you make a good point that we could just use @ instead of self.. I'm willing to make the change, but I'd like to see if there are any other thoughts about this.

Contributor

mxie replied Mar 28, 2013

I think it makes sense for us to remove uses of instance variables and use barewords instead to make their implementations transparent. However, I do think that for assignments, you make a good point that we could just use @ instead of self.. I'm willing to make the change, but I'd like to see if there are any other thoughts about this.

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