Make the order of Hash#reverse_merge! consistent with HashWithIndifferentAccess

[Erol]

Summary

Currently, the result of Hash#reverse_merge! deviates from the ordering of similar methods:

ab = {a: 1, b: 2}
c = {c: 3}

indifferent_ab = HashWithIndifferentAccess.new(a: 1, b: 2)
indifferent_c = HashWithIndifferentAccess.new(c: 3)

ab.reverse_merge(c) #=> {c: 3, a: 1, b: 2}
ab.reverse_merge!(c) #=> {a: 1, b: 2, c: 3}
indifferent_ab.reverse_merge(c) #=> {c: 3, a: 1, b: 2}
indifferent_ab.reverse_merge!(c) #=> {c: 3, a: 1, b: 2}

This PR aims to make the element order consistent across the 4 methods such that:

ab.reverse_merge!(c) #=> {c: 3, a: 1, b: 2}

[rails-bot]

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

[al2o3cr]

Probably not a concern in any realistic use cases for this function, but this change does make an extra copy of the original Hash compared to the previous implementation.

[Erol]

That's true. What I also haven't done yet is run a benchmark to see the impact.

It does make the implementation simpler to maintain, since it's the same as HashWithIndifferentAccess:

rails/activesupport/lib/active_support/hash_with_indifferent_access.rb

Line 230 in 71da390

def reverse_merge!(other_hash)

A further improvement could be to remove the method definition there, since it inherits from Hash.

[Erol]

What do you think of this change @sgrif?

[Erol]

Hi @maclover7, anyone I can consult this with?

[Erol]

Hi @rafaelfranca just wanted to consult with you about this change. If this is something that can be merged, I can have the PR rebased

[Erol]

Just saw this, thanks @sgrif