Fix handling of default proc values in Mash #259

Merged
merged 1 commit into from Dec 29, 2014

Conversation

Projects
None yet
2 participants
@Erol
Contributor

Erol commented Dec 29, 2014

This fixes #257.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Dec 29, 2014

Contributor

This only fixes the emtpy scenario, no? What happens when the array already has elements?

Contributor

dblock commented Dec 29, 2014

This only fixes the emtpy scenario, no? What happens when the array already has elements?

CHANGELOG.md
@@ -5,6 +5,7 @@
* [#251](https://github.com/intridea/hashie/pull/251): Add block to indifferent access #fetch - [@jgraichen](https://github.com/jgraichen).
* [#252](https://github.com/intridia/hashie/pull/252): Add support for conditionally required Hashie::Dash attributes - [@ccashwell](https://github.com/ccashwell).
* [#256](https://github.com/intridia/hashie/pull/256): Inherit key coercions - [@Erol](https://github.com/Erol).
+* [#259](https://github.com/intridia/hashie/pull/259): Fix default array values - [@Erol](https://github.com/Erol).

This comment has been minimized.

@dblock

dblock Dec 29, 2014

Contributor

This doesn't really explain what the change is, start by saying where this is (in a Mash).

@dblock

dblock Dec 29, 2014

Contributor

This doesn't really explain what the change is, start by saying where this is (in a Mash).

spec/hashie/mash_spec.rb
@@ -400,6 +400,8 @@ class SubMash < Hashie::Mash
expect(initial.default).to be_nil
expect(initial.test).to eq []
expect(initial.test?).to be_truthy
+ initial.hello << 100

This comment has been minimized.

@dblock

dblock Dec 29, 2014

Contributor

This could use separate specs, and definitely one closer to the code in the bug referenced.

@dblock

dblock Dec 29, 2014

Contributor

This could use separate specs, and definitely one closer to the code in the bug referenced.

@Erol Erol changed the title from Fix default array values to Fix handling of default proc values in Mash Dec 29, 2014

@Erol

This comment has been minimized.

Show comment
Hide comment
@Erol

Erol Dec 29, 2014

Contributor

You're right, the previous fix only handled empty arrays. Hashes are also bugged - empty or not:

mash = Hashie::Mash.new { |h, k| h[k] = {} }
mash.foo[:bar] = 'baz'
mash.inspect #=> #<Hashie::Mash foo=#<Hashie::Mash>>

I've rewritten the fix to handle empty/non-empty arrays and hashes. I also did a rebase.

Contributor

Erol commented Dec 29, 2014

You're right, the previous fix only handled empty arrays. Hashes are also bugged - empty or not:

mash = Hashie::Mash.new { |h, k| h[k] = {} }
mash.foo[:bar] = 'baz'
mash.inspect #=> #<Hashie::Mash foo=#<Hashie::Mash>>

I've rewritten the fix to handle empty/non-empty arrays and hashes. I also did a rebase.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Dec 29, 2014

Contributor

How does this work? :)

Fix RuboCop please.

Contributor

dblock commented Dec 29, 2014

How does this work? :)

Fix RuboCop please.

@Erol

This comment has been minimized.

Show comment
Hide comment
@Erol

Erol Dec 29, 2014

Contributor

Okay, I made a few more adjustments to the fix. It should now also handle hash-type accessors:

mash = Hashie::Mash.new { |h, k| h[k] = [] }
mash[:hello] << 100

How this works: The default proc sets a key's initial value, but it's return value is also immediately returned. But Mash is converting values by default, and the object returned by the default proc is different from what is actually persisted. This is apparent if we fetch a key's value twice and inspect their object IDs:

mash = Hashie::Mash.new { |h, k| h[k] = [] }
mash.hello.object_id #=> 10101020 # default_proc
mash.hello.object_id #=> 10101040 # converted and persisted

What this updated fix does is to call the default proc first if the key has not been set, and then fetch the actual key's persisted value instead of relying on the default proc's return value.

I also got the RuboCop warning fixed. Although it looks like we're already on the threshold in terms of LoC for Mash. :(

Contributor

Erol commented Dec 29, 2014

Okay, I made a few more adjustments to the fix. It should now also handle hash-type accessors:

mash = Hashie::Mash.new { |h, k| h[k] = [] }
mash[:hello] << 100

How this works: The default proc sets a key's initial value, but it's return value is also immediately returned. But Mash is converting values by default, and the object returned by the default proc is different from what is actually persisted. This is apparent if we fetch a key's value twice and inspect their object IDs:

mash = Hashie::Mash.new { |h, k| h[k] = [] }
mash.hello.object_id #=> 10101020 # default_proc
mash.hello.object_id #=> 10101040 # converted and persisted

What this updated fix does is to call the default proc first if the key has not been set, and then fetch the actual key's persisted value instead of relying on the default proc's return value.

I also got the RuboCop warning fixed. Although it looks like we're already on the threshold in terms of LoC for Mash. :(

@Erol

This comment has been minimized.

Show comment
Hide comment
@Erol

Erol Dec 29, 2014

Contributor

I also had to rewrite Mash.load. The current implementation has a conflict with the fix.

Contributor

Erol commented Dec 29, 2014

I also had to rewrite Mash.load. The current implementation has a conflict with the fix.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Dec 29, 2014

Contributor

Fyi, for RuboCop you can rerun with --auto-gen-config to update the TODO file.

Contributor

dblock commented Dec 29, 2014

Fyi, for RuboCop you can rerun with --auto-gen-config to update the TODO file.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Dec 29, 2014

Contributor

Merging, this is excellent work, thanks.

Contributor

dblock commented Dec 29, 2014

Merging, this is excellent work, thanks.

dblock added a commit that referenced this pull request Dec 29, 2014

Merge pull request #259 from Erol/fix-default-array-values
Fix handling of default proc values in Mash

@dblock dblock merged commit 337a980 into intridea:master Dec 29, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@Erol Erol deleted the Erol:fix-default-array-values branch Dec 30, 2014

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