Skip to content
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

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

Merged
merged 1 commit into from Sep 5, 2012
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions activesupport/CHANGELOG.md
@@ -1,5 +1,11 @@
## Rails 4.0.0 (unreleased) ##

* An optional block can be passed to `HashWithIndifferentAccess#update` and `#merge`.
The block will be invoked for each duplicated key, and used to resolve the conflict,
thus replicating the behaviour of the corresponding methods on the `Hash` class.

*Leo Cassarani*

* Remove `j` alias for `ERB::Util#json_escape`.
The `j` alias is already used for `ActionView::Helpers::JavaScriptHelper#escape_javascript`
and both modules are included in the view context that would confuse the developers.
Expand Down
27 changes: 21 additions & 6 deletions activesupport/lib/active_support/hash_with_indifferent_access.rb
Expand Up @@ -95,10 +95,10 @@ def []=(key, value)

alias_method :store, :[]=

# Updates the receiver in-place merging in the hash passed as argument:
# Updates the receiver in-place, merging in the hash passed as argument:
#
# hash_1 = ActiveSupport::HashWithIndifferentAccess.new
# hash_2[:key] = "value"
# hash_1[:key] = "value"
#
# hash_2 = ActiveSupport::HashWithIndifferentAccess.new
# hash_2[:key] = "New Value!"
Expand All @@ -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}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Thanks

#
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably cache the convert_key(key) method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self
end
end
Expand Down Expand Up @@ -173,8 +188,8 @@ def dup
# This method has the same semantics of +update+, except it does not
# modify the receiver but rather returns a new hash with indifferent
# access with the result of the merge.
def merge(hash)
self.dup.update(hash)
def merge(hash, &block)
self.dup.update(hash, &block)
end

# Like +merge+ but the other way around: Merges the receiver into the
Expand Down
23 changes: 23 additions & 0 deletions activesupport/test/core_ext/hash_ext_test.rb
Expand Up @@ -428,6 +428,29 @@ 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]

other_indifferent = HashWithIndifferentAccess.new('a' => 9, :b => 2)

merged = hash.merge(other_indifferent) { |key, old, new| old + new }

assert_equal HashWithIndifferentAccess, merged.class
assert_equal 10, merged[:a]
assert_equal 5, merged[:b]
end

def test_indifferent_reverse_merging
hash = HashWithIndifferentAccess.new('some' => 'value', 'other' => 'value')
hash.reverse_merge!(:some => 'noclobber', :another => 'clobber')
Expand Down