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

Unexpected [] behavior compared to other access patterns #25

Closed
adsteel opened this issue Apr 19, 2022 · 4 comments · Fixed by #26 or #29
Closed

Unexpected [] behavior compared to other access patterns #25

adsteel opened this issue Apr 19, 2022 · 4 comments · Fixed by #26 or #29

Comments

@adsteel
Copy link
Contributor

adsteel commented Apr 19, 2022

For top level keys you can use symbols, strings, or dot notation:

> Setting[:admin_auth]
=> {"url"=>"http://localhost:5100", "secret"=>"..."}
> Setting['admin_auth']
=> {"url"=>"http://localhost:5100", "secret"=>"..."}
> Setting.admin_auth
=> {"url"=>"http://localhost:5100", "secret"=>"..."}

For nested keys can use symbols-in-brackets, but not strings-in-brackets

> Setting.admin_auth[:url]
=> nil # surprise!
> Setting.admin_auth['url']
=> "http://localhost:5050"

If you call the top level key like a method, you can use symbols or strings as the argument

> Setting.admin_auth(:url)
=> "http://localhost:5050"
> Setting.admin_auth('url')
=> "http://localhost:5050"

You cannot call nested keys like methods

> Setting.admin_auth.url
NoMethodError: undefined method `url'

The "gotcha" here is using brackets with symbols, which could surprisingly not return a value. There are a few ways to address this inconsistency, but given the existing functionality that already works with both string and symbol, I'd suggest we recursively modify the Setting object so that all [] methods behave the same.

  • Suggestion 1. At least all nested keys should respond to both [sym|string] access equally
  • Suggestion 2. All keys should respond to the same methods equally, i.e. make all keys act like top level keys by responding to dot notation, method calls formation, and [string|sym] access.

Suggestion 2 is the most symmetrical, but also will require more monkey patching of Ruby core functionality. I'm leaning toward suggestion 1.

Suggestions 2 could be done by wrapping each key recursively with Hashie::Mash, and might be the easiest.

Once we have agreement on the approach I can work on a fix.

@mboeh
Copy link
Contributor

mboeh commented Apr 19, 2022

but also will require more monkey patching of Ruby core functionality.

Could you expand on this a little? Fittings could return a Hashie::Mash or something along those lines, which shouldn't require any monkeypatching.

(My personal vote would be for deprecating Fittings, which is more trouble than it's worth, but I understand that's another question.)

@adsteel
Copy link
Contributor Author

adsteel commented Apr 19, 2022

@mboeh Ah, I was not familiar with that gem. Looks like that's what the top level is wrapped in mirrors top level behavior, so if we recursively wrapped all keys with Hashie::Mash that could be a good option.

@adsteel
Copy link
Contributor Author

adsteel commented Apr 19, 2022

It does look like there's functionality similar to Hashie::Mash implemented with separate logic for the top level keys, so I do wonder if we should also move to wrap the top level keys with Hashie::Mash, so all keys behave exactly the same.

@mboeh
Copy link
Contributor

mboeh commented Apr 19, 2022

I think that would be the least surprising solution. In any of these cases, we'll probably need to make it a new major version, though, in case anybody has code out there assuming that these methods return regular Hashes (I very well might have some myself).

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