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

Already on GitHub? Sign in to your account

Avoid Mutation of @data from external reference #51

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

kungfumike commented Mar 14, 2013

return a mutable reference that allowed in memory mutation of the
@DaTa struct inside MockRedis

Spec is probably not consistent with present spec structure but
does illustrate a red/green against method. Spec might also need
be moved as it effects more then just hgetall.

This commit done on the dime of:
Simply Measured
twitter: @simplymeasured
web: http://www.simplymeasured.com

@kungfumike kungfumike Added a trinay and .dup to the return of 'with_thing_at' to avoid
return a mutable reference that allowed in memory mutation of the
@data struct inside MockRedis

Spec is probably not consistent with present spec structure but
does illustrate a red/green against method. Spec might also need
be moved as it effects more then just hgetall.

This commit done on the dime of:
Simply Measured
twitter: @SimplyMeasured
web: http://www.simplymeasured.com
c19d634

The fix looks OK, but this isn't quite ready to merge.

First of all, what's a "trinay"? Also, there's a typo in the 'it assertion: 'mutatble' -> 'mutable'.

Would you also please move it above the shared example invocation like the rest of the specs? (line 20).

I'm also a little unclear on the need for the ternary in general. If you're worried about calling #dup on a NilClass, you could do:

ret.dup if ret

which evaluates to nil if ret is nil itself.

Thanks,
Aiden

@kungfumike kungfumike Missed a MASSIVE set of failures in the sadd spec the first time
arround. This corrals the dump call to arrays hashes and strings
15d8f44
Contributor

kungfumike commented Mar 14, 2013

I can move that stuff around, sure.

Trinay is what happens when you try to type 'ternary' high on cold medicine.

I had to further correct this logic to only apply to hashes, arrays, and strings. As certian methods like sadd return 'true' which like nil also can't be duped.

I will get those specs moved around

Contributor

kungfumike commented Mar 14, 2013

@sectioneight Moved the spec around for you, go ahead and give it another look when you have a chance. Thanks.

Contributor

sectioneight commented Mar 14, 2013

@kungfumike Thanks! We're a little, uh... obsessive about our code style at @causes. If you don't mind, I'll squash these all into one commit, since we don't want any single point in history to have failing specs.

Contributor

sectioneight commented Mar 14, 2013

Cherry-pick to master at a08edf7 courtesy of our Gerrit code review instance. Thanks for fixing this up!

@sds sds added the bug label Apr 8, 2014

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