Wrap YAML in Hashie::Mash #26
Conversation
`Setting` had different behavior depending on the access pattern.
Sometimes it was more forgiving than Ruby, other times it was not.
```ruby
Setting[:foo] # bar
Setting['foo'] # bar
Setting.foo('bar') # baz
Setting.foo(:bar) # baz
Setting.foo['bar'] # baz
Setting.foo[:bar] # nil
```
By wrapping the YAML in Hashie::Mashie, we can avoid breaking any
existing tests while improving the behavior consistency.
| @@ -20,6 +20,7 @@ Gem::Specification.new do |s| | |||
| s.executables = s.files.grep(%r{^exe/}) { |f| File.basename(f) } | |||
| s.require_paths = ["lib"] | |||
|
|
|||
| s.add_dependency "hashie" | |||
There was a problem hiding this comment.
I'm hesitant to add this dependency to every app that uses fittings. Is this bug something that can be fixed without it?
There was a problem hiding this comment.
It can be, yes. We would have to either globally monkey patch Hash or recursively wrap each object in an enhanced hash, which is essentially what Hashie::Mash is doing for us. The tradeoffs with those approaches being that we avoid a new dependency, but incur supporting similar complex logic on our own (the recursive logic) or extend our monkey patching of a core Rails class. I personally think we should avoid monkey patching Ruby classes due to the risk that can bring. Too much depends on them for us to be tweaking them on our own.
I can see the arguments for the recursive option and this option. I lean toward this approach because it minimizes our future support needs for this gem, but I'm definitely open on the issue.
There was a problem hiding this comment.
Just a data point: per appetizer, it looks like every app that uses fittings already uses hashie as well. There may be a flaw in my method, posting here for review/scrutiny:
appetizer -g hashie | jq -r keys[] > hashie
appetizer -g fittings | jq -r keys[] > fittings
comm -23 fittings hashie
# no output, meaning no lines unique to "fittings"
There was a problem hiding this comment.
Ok. I don't want to block progress here. I've seen dependencies prevent upgrades to Rails and Ruby. And every dependency is an extra download for everyone.
But if everyone already needs it... this is probably fine.
There was a problem hiding this comment.
Sorry to be giving late feedback—I hinted at this in slack, but api_client gem will always pull in hashie as a dependency (for backwards-compatibility with code that was written with the version that used hashie), but it's very common to configure subclasses or other uses to use plain hashes instead, so teams are able to opt-in to Hashie's performance impact.
That's not to say there aren't apps actually using hashie, but I don't think appetizer is answering the right question for us.
|
Right, it's a separate task to update the version and release. See https://github.com/stitchfix/eng-wiki/blob/main/technical-topics/updating-gem-versions.md and https://github.com/stitchfix/eng-wiki/blob/main/technical-topics/gem-changes-with-new-feature.md. |
Resolves #25
Problem
Settinghad different behavior depending on the access pattern. Sometimes it was more forgiving than Ruby, other times it was not.Solution
By wrapping the YAML in Hashie::Mashie, we can avoid breaking any existing tests while improving the behavior consistency. Because this may break how folks are using the gem, it should be released as a breaking change.