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

Mash #[]= does not behave like Hash's #390

Open
schnittchen opened this issue Jan 17, 2017 · 12 comments
Open

Mash #[]= does not behave like Hash's #390

schnittchen opened this issue Jan 17, 2017 · 12 comments
Labels

Comments

@schnittchen
Copy link

schnittchen commented Jan 17, 2017

I hope this demonstrates the problem:

irb(main):001:0> require 'hashie'
=> true
irb(main):002:0> Hashie::Mash
=> Hashie::Mash
irb(main):003:0> class H < Hash; end
=> nil
irb(main):004:0> h = H.new
=> {}
irb(main):005:0> hashie = Hashie::Mash.new
=> #<Hashie::Mash>
irb(main):006:0> hashie[:key] = h
=> {}
irb(main):007:0> hashie[:a_key] = h
=> {}
irb(main):008:0> hashie[:a_key].equal?(h)
=> false
irb(main):009:0> hash = {}
=> {}
irb(main):010:0> hash[:a_key] = h
=> {}
irb(main):011:0> hash[:a_key].equal?(h)
=> true

In fact, hashie contains a Hash value instead of a H.

@schnittchen
Copy link
Author

#265 might be related ;)

@michaelherold
Copy link
Member

Could you please make a failing test and submit that as a pull request? That will help us diagnose what you're running into faster.

@schnittchen
Copy link
Author

schnittchen commented Jan 17, 2017

Sure, glad to help. See schnittchen@4b7ed0c
Shall I split into two examples?

@dblock
Copy link
Member

dblock commented Jan 18, 2017

Yes, and make a PR. Maybe even with a fix?

@dblock dblock added the bug? label Jan 18, 2017
@schnittchen
Copy link
Author

I'l see what I can do. This is indeed considered a bug, then? Have you exercised my test btw?

@dblock
Copy link
Member

dblock commented Jan 18, 2017

Actually maybe not. You get a Mash? A Hash? Now that I'm looking at it I think we either convert subhashes into Mash or leave them alone, otherwise you can't carry mash-like behavior deeper into it. Am I wrong?

@schnittchen
Copy link
Author

That's in conflict with https://github.com/intridea/hashie/blob/master/README.md#mash "while still retaining all Hash functionality"

@michaelherold
Copy link
Member

All Hash functionality is still there. All that means is that your injected item still responds to all of the Hash methods. We don't make any promises about subclasses of Hash that extend that capability.

@schnittchen
Copy link
Author

IMHO it's not even about Hash. It's a very basic container contract: I'll find what I've put in.

@dblock
Copy link
Member

dblock commented Jan 20, 2017

To Mash's defense it's designed to chew data. Feel free to try and fix this, we can look at a PR and think about the implications.

@schnittchen
Copy link
Author

schnittchen commented Jan 29, 2017

I understand that the idea is to have

mhash[:foo] = { bar: 3 }
mhash.foo.bar = 5

which implies some conversion in []=.

The only alternatives are

  • extend mashie-like behaviour onto the singleton that's being stored. sounds like a terrible idea
  • make hash detection stricter, by using instance_of.

@michaelherold
Copy link
Member

This is related to #432 and #434.

I don't think that this is a change that we can make because it breaks the behavior of accessing deeply-nested Mashes. I'm not sure if there's a way we could work around that. Perhaps if we reworked Mash to use Hash#dig and wrapped the results in a proxy object, or something, but that would be a massive undertaking, I think.

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

No branches or pull requests

3 participants