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

ActiveSupport's Hash#reverse_merge doesn't get along with Hashie::Mash #270

Closed
ajvondrak opened this issue Jan 29, 2015 · 2 comments · Fixed by #281
Closed

ActiveSupport's Hash#reverse_merge doesn't get along with Hashie::Mash #270

ajvondrak opened this issue Jan 29, 2015 · 2 comments · Fixed by #281

Comments

@ajvondrak
Copy link

I don't know how much of a concern ActiveSupport compatibility is for you, but I thought that this behavior was surprising:

[1] pry(main)> require 'hashie'
=> true
[2] pry(main)> require 'active_support/all'
=> true
[3] pry(main)> params = Hashie::Mash.new(a: 1, b: 2)
=> {"a"=>1, "b"=>2}
[4] pry(main)> params[:a]
=> 1
[5] pry(main)> params['a']
=> 1
[6] pry(main)> params.merge(a: 2)
=> {"a"=>2, "b"=>2}
[7] pry(main)> params.reverse_merge(a: 2)
=> {:a=>2, "a"=>1, "b"=>2}

This is of course a consequence of ActiveSupport defining reverse_merge as

def reverse_merge(other_hash)
  other_hash.merge(self)
end

Here, other_hash is of class Hash, so of course Hash#merge doesn't play with Hashie::Mash's key conversion. Not sure how you'd fix it efficiently (Hash#merge being an MRI builtin), but maybe monkeypatch in a Hashie::Mash#reverse_merge like

def reverse_merge(other_hash)
  other_hash.each do |key, value|
    self[key] ||= value
  end
end

Doesn't capture block_given? behavior or anything, though.

Another workaround for now is just

[8] pry(main)> params.merge(a: 2) { |key, old, new| old }
=> {"a"=>1, "b"=>2}
[9] pry(main)> params.merge(a: 2, c: 3) { |key, old, new| old }
=> {"a"=>1, "b"=>2, "c"=>3}

or doing the ||=s by hand.

@ajvondrak
Copy link
Author

Looking at the source for Hashie::Mash now, I realize that you're already defining a custom #merge method, so...seems like #reverse_merge isn't too far of a stretch. Could even just do it like this:

def reverse_merge(other_hash)
  Hashie::Mash.new(other_hash).merge(self)
end

@dblock
Copy link
Member

dblock commented Jan 30, 2015

We generally like not to have surprises with ActiveSupport. So feel free to PR this, and thank you.

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

Successfully merging a pull request may close this issue.

2 participants