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

Make the order of Hash#reverse_merge! consistent with HashWithIndifferentAccess #28077

Merged
merged 4 commits into from Jul 17, 2017

Conversation

Projects
None yet
5 participants
@Erol
Contributor

Erol commented Feb 20, 2017

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

This comment has been minimized.

rails-bot commented Feb 20, 2017

r? @sgrif

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

@al2o3cr

This comment has been minimized.

Contributor

al2o3cr commented Feb 20, 2017

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

This comment has been minimized.

Contributor

Erol commented Feb 20, 2017

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:

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

@Erol

This comment has been minimized.

Contributor

Erol commented Mar 3, 2017

What do you think of this change @sgrif?

Erol added some commits Feb 20, 2017

@Erol

This comment has been minimized.

Contributor

Erol commented Mar 15, 2017

Hi @maclover7, anyone I can consult this with?

@Erol

This comment has been minimized.

Contributor

Erol commented Apr 9, 2017

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

@sgrif sgrif merged commit 3bda7ff into rails:master Jul 17, 2017

1 check was pending

codeclimate Code Climate is analyzing this code.
Details

@Erol Erol deleted the Erol:make-reverse-merge-bang-order-consistent branch Jul 19, 2017

@Erol

This comment has been minimized.

Contributor

Erol commented Jul 19, 2017

Just saw this, thanks @sgrif

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