Add an optional block to HashWithIndifferentAccess#update and #merge #7519

Merged
merged 1 commit into from Sep 5, 2012

Conversation

Projects
None yet
3 participants
Contributor

leocassarani commented Sep 4, 2012

When a block is passed into HashWithIndifferentAccess#update or #merge, it will be invoked for each duplicated key with the key in question, the value for the key in the receiver, and the value in the hash being merged. The return value of the block will be used to resolve the conflict:

hash_1 = ActiveSupport::HashWithIndifferentAccess.new
hash_1[:key] = 10

hash_2 = ActiveSupport::HashWithIndifferentAccess.new
hash_2['key'] = 20

hash_1.merge(hash_2) { |key, old, new| old + new } # => {"key"=>30}

This is consistent with long-standing behaviour of the Standard Library's Hash class. Indeed, without this change, it would not be immediately obvious to someone passing a block into HashWithIndifferentAccess#merge that its behaviour doesn't reflect Hash#merge, as the block will be silently ignored.

@carlosantoniodasilva carlosantoniodasilva commented on the diff Sep 4, 2012

...rt/lib/active_support/hash_with_indifferent_access.rb
def update(other_hash)
if other_hash.is_a? HashWithIndifferentAccess
super(other_hash)
else
- other_hash.each_pair { |key, value| regular_writer(convert_key(key), convert_value(value)) }
+ other_hash.each_pair do |key, value|
+ if block_given? && key?(key)
+ value = yield(convert_key(key), self[key], value)
+ end
+ regular_writer(convert_key(key), convert_value(value))
+ end
@carlosantoniodasilva

carlosantoniodasilva Sep 4, 2012

Owner

You could probably cache the convert_key(key) method.

@leocassarani

leocassarani Sep 4, 2012

Contributor

Yeah, I did consider it but a) it will make the each_pair block one line longer b) it's a very inexpensive method to call (check if argument is a Symbol, call #to_s on it if it is). But if you think I should add key = convert_key(key) before the if statement, I'm perfectly happy to do it.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Sep 4, 2012

activesupport/test/core_ext/hash_ext_test.rb
@@ -428,6 +428,21 @@ def test_indifferent_merging
assert_equal 2, hash['b']
end
+ def test_indifferent_merging_with_block
+ hash = HashWithIndifferentAccess.new
+ hash[:a] = 1
+ hash['b'] = 3
+
+ other = { 'a' => 4, :b => 2, 'c' => 10 }
+
+ merged = hash.merge(other) { |key, old, new| old > new ? old : new }
+
+ assert_equal HashWithIndifferentAccess, merged.class
+ assert_equal 4, merged[:a]
+ assert_equal 3, merged['b']
+ assert_equal 10, merged[:c]
+ end
@carlosantoniodasilva

carlosantoniodasilva Sep 4, 2012

Owner

Perhaps you'd need to add another test to check merging a HWIA to another HWIA, where the super call would take place right?

@leocassarani

leocassarani Sep 4, 2012

Contributor

You're absolutely right. 7cbe52e adds another test case for it.

@rafaelfranca rafaelfranca commented on the diff Sep 4, 2012

...rt/lib/active_support/hash_with_indifferent_access.rb
@@ -110,12 +110,27 @@ def []=(key, value)
# In either case the merge respects the semantics of indifferent access.
#
# If the argument is a regular hash with keys +:key+ and +"key"+ only one
- # of the values end up in the receiver, but which was is unespecified.
+ # of the values end up in the receiver, but which one is unspecified.
+ #
+ # When given a block, the value for duplicated keys will be determined
+ # by the result of invoking the block with the duplicated key, the value
+ # in the receiver, and the value in +other_hash+. The rules for duplicated
+ # keys follow the semantics of indifferent access:
+ #
+ # hash_1[:key] = 10
+ # hash_2['key'] = 12
+ # hash_1.update(hash_2) { |key, old, new| old + new } # => {"key"=>22}
@rafaelfranca

rafaelfranca Sep 4, 2012

Owner

Could you put a space after the { and before the }. Like this { "key" => 22 }

@leocassarani

leocassarani Sep 4, 2012

Contributor

While that's the amount of whitespace I would use in my own code when declaring a hash literal, the snippet you're referring to is intended to represent the value of the expression preceding it (note the # =>). Omitting surrounding whitespace is consistent with what IRB would print out, and indeed with the syntax used throughout the rest of the file: see lines 30, 106, and 200.

@rafaelfranca

rafaelfranca Sep 5, 2012

Owner

Agree. Thanks

Owner

rafaelfranca commented Sep 4, 2012

Could you squash your commits please?

@leocassarani leocassarani Extend HashWithIndifferentAccess#update to take an optional block
When a block is passed into the method, it will be invoked for each
duplicated key, with the key in question and the two values as
arguments. The value for the duplicated key in the receiver will
be set to the return value of the block.

This behaviour matches Ruby's long-standing implementation of
Hash#update and is intended to provide a more consistent interface.

HashWithIndifferentAccess#merge is also affected by the change, as it
uses #update internally.
edab820
Contributor

leocassarani commented Sep 5, 2012

@rafaelfranca I have squashed my commits into a single one (edab820). Thanks for pointing that out.

rafaelfranca merged commit ac5089d into rails:master Sep 5, 2012

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