Changes to `Mash` initialization key string conversion. #521

carolineartz · 2020-04-24T19:24:41Z

As Mash is conceived to provide psuedo-object functionality, handling keys which cannot represent a method call falls outside the scope of provided value.

This PR changes the initialization behavior of Mash to convert keys to strings only when the key is a candidate for indifferent (string/symbol) access. That is, non-string/symbol keys such as 1, ["1"] {foo: "bar"} will be left as is.

Before this PR, we would see:

[1] pry(main)> Hashie::Mash.new({foo: "bar"} => "baz", "1" => "one string", :"1" => "one sym", 1 => "one num")
=> {"{:foo=>\"bar\"}"=>"baz", "1"=>"one num"}

After this PR, we see:

[1] pry(main)> Hashie::Mash.new({foo: "bar"} => "baz", "1" => "one string", :"1" => "one sym", 1 => "one num")
=> {{:foo=>"bar"}=>"baz", "1"=>"one sym", 1=>"one num"}

Bug Fix

Discussed in issue #516, the changes here address an existing bug where any Hash extended with Hashie::Extensions::SymbolizeKeys or Mash extended with Hashie::Extensions::Mash::SymbolizeKeys will throw an error when symbolizing keys if the object has any keys which cannot be converted to symbols, e.g., numeric keys.

Given the changes introduced in Mash initialization it is now logical that any key which does not respond to to_sym is preserved in its original type when symbolizing via either Hashie extension. This behavior is intended to mirror that of ActiveSupport's core Hash extensions [deep]_symbolize_keys[!].

TODO:

Get review on new changes to source
Update README to make this behavior clear.
Add to UPGRADING.md describing the change(s)
Bump version

…ing/symbol keys. Failed examples: rspec ./spec/hashie/extensions/mash/symbolize_keys_spec.rb:33 # Hashie::Extensions::Mash::SymbolizeKeys implicit to_hash on double splat is converted on method calls rspec ./spec/hashie/extensions/mash/symbolize_keys_spec.rb:37 # Hashie::Extensions::Mash::SymbolizeKeys implicit to_hash on double splat is converted on explicit operator call rspec ./spec/hashie/extensions/symbolize_keys_spec.rb:98 # Hashie::Extensions::SymbolizeKeys singleton methods .symbolize_keys does not raise error rspec ./spec/hashie/extensions/symbolize_keys_spec.rb:101 # Hashie::Extensions::SymbolizeKeys singleton methods .symbolize_keys produces expected symbolized hash rspec ./spec/hashie/extensions/symbolize_keys_spec.rb:106 # Hashie::Extensions::SymbolizeKeys singleton methods .symbolize_keys! does not raise error rspec ./spec/hashie/extensions/symbolize_keys_spec.rb:109 # Hashie::Extensions::SymbolizeKeys singleton methods .symbolize_keys! produces expected symbolized hash

Failed examples: rspec ./spec/hashie/extensions/mash/symbolize_keys_spec.rb:33 # Hashie::Extensions::Mash::SymbolizeKeys implicit to_hash on double splat is converted on method calls rspec ./spec/hashie/extensions/mash/symbolize_keys_spec.rb:37 # Hashie::Extensions::Mash::SymbolizeKeys implicit to_hash on double splat is converted on explicit operator call

Failed examples: rspec ./spec/hashie/mash_spec.rb:902 # Hashie::Mash#invert returns a mash with the keys and values inverted rspec ./spec/hashie/mash_spec.rb:982 # Hashie::Mash#dig with numeric key accepts a numeric value as key

lib/hashie/extensions/mash/symbolize_keys.rb

dblock

Solid work! Looking forward to this being merged, thanks for hanging in here.

For CHANGELOG, what do you think of "Do not convert keys that cannot be represented as symbols to String in Mash initialization"?

TODO:

Update README to make this behavior clear.
Add to UPGRADING.md describing the change(s)
Bump version
Fix build (if the current failure is unrelated, make a separate PR first)

CHANGELOG.md

lib/hashie/extensions/mash/symbolize_keys.rb

lib/hashie/mash.rb

carolineartz · 2020-04-27T17:53:15Z

@dblock what can be done about this https://travis-ci.org/github/hashie/hashie/jobs/680206889. Not clear for me whats the root of the failure so I can fix it.

dblock · 2020-04-27T22:06:10Z

@dblock what can be done about this https://travis-ci.org/github/hashie/hashie/jobs/680206889. Not clear for me whats the root of the failure so I can fix it.

That's this (see above):

Fix the CHANGELOG and it will go away and turn green. They both need a period at the end of the line at least.

README.md

spec/hashie/mash_spec.rb

dblock

This is good with me to merge with a proper UPGRADING entry, please @carolineartz.

@michaelherold Anything from you?

CHANGELOG.md

carolineartz · 2020-05-04T19:32:59Z

@dblock I didn't do the version bump itself... wasn't sure if this was to be released or if additional changes are going to be added the next release. LMK if i should.

dblock · 2020-05-04T20:24:27Z

@dblock I didn't do the version bump itself... wasn't sure if this was to be released or if additional changes are going to be added the next release. LMK if i should.

You should. Bump it.

dblock

I am ready to merge this with version bump. @michaelherold

CHANGELOG.md

dblock · 2020-05-04T20:26:31Z

UPGRADING.md

@@ -1,6 +1,44 @@
 Upgrading Hashie
 ================

+### Upgrading to 5.0.0


I wonder whether we should do 4.1.0 for this. Some elements of the behavior change, but not major enough for API? @michaelherold what do you prefer?

i was wavering on suggesting this...but it is not technically inconsistent with Semver to have entries in changed that don't correspond with a major version bump?

What do I know? :) We've always treated the second digit as "major". We'd be at 24.0 otherwise and that feels like a lot. I'm open to this debate, but for now I say let's do 4.2.0. Will you make this change here @carolineartz, please?

Actually .. re-reading what @michaelherold added to CHANGELOG I'll change my mind. Merging as is.

Being strict with semver is always hard, especially when folks expect new features in the Major versions. I can agree with the bump to 5.0.0 if we've changed expected behavior in a way that could be breaking.

We did. Amen.

dblock · 2020-05-05T01:15:57Z

Merged. Thanks @carolineartz for hanging in here for a while and making this happen.

carolineartz added 14 commits April 23, 2020 16:28

Use respond_to instead of rescue NoMethodError.

44c2aad

Mash#invert returns a properly inverted mash.

6386222

Adds spec for Mash initialization key type conversion.

fca3482

Update method documentation.

1e57f3b

Make test assertion for stringifying keys more specific.

f372cdc

Adds Mash spec for preserving key types which cannot be symbolized.

c2bb200

Remove moot spec for dig access with numeric key.

4d2c20c

Add Mash initialization spec with integer, symbol and string types.

aecd9a6

Fix typo in Mash SymbolizeKeys extension docs.

3fdf130

Add Misc entry in changelog for docs typo fix.

bfcf4e6

Add TODO for change description.

f8a5ac1

carolineartz force-pushed the fix-symbolize-keys branch from 81d46af to f8a5ac1 Compare April 24, 2020 19:27

This comment has been minimized.

Sign in to view

carolineartz commented Apr 24, 2020

View reviewed changes

lib/hashie/extensions/mash/symbolize_keys.rb Show resolved Hide resolved

dblock requested changes Apr 25, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

lib/hashie/extensions/mash/symbolize_keys.rb Show resolved Hide resolved

lib/hashie/mash.rb Outdated Show resolved Hide resolved

carolineartz added 2 commits April 27, 2020 12:15

Remove non-symbolizable keys from duoble splat specs.

da9f81c

Revert unnecessary formatting change.

a8bc220

This comment has been minimized.

Sign in to view

carolineartz added 3 commits April 30, 2020 13:38

Change Hashie::Hash for consistent symbolize keys behavior.

2fae965

Refactor Mash#custom_writer for clarity.

09e0f25

Update README.

8cea5be

carolineartz commented Apr 30, 2020

View reviewed changes

README.md Show resolved Hide resolved

carolineartz added 2 commits April 30, 2020 16:15

Remove the article 'a' from Mash reference.

851e7d0

Update Changelog.

7782e0c

carolineartz force-pushed the fix-symbolize-keys branch from 8175849 to 7782e0c Compare April 30, 2020 21:25

carolineartz commented Apr 30, 2020

View reviewed changes

spec/hashie/mash_spec.rb Show resolved Hide resolved

dblock requested changes May 1, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

carolineartz added 4 commits May 4, 2020 13:25

Revert removal of 'Your contribution here.' from Changelog.

90c8764

Format types as code in Changelog.

e4fd4e0

Add entries to Upgrading.

5a29ed4

Add entry in changelog for fix for issue hashie#516.

54284ee

carolineartz requested a review from dblock May 4, 2020 19:30

dblock requested changes May 4, 2020

View reviewed changes

carolineartz added 2 commits May 4, 2020 16:02

Bump version to 5.0.0.

d816177

Remove from Changelog.

536c3fa

dblock merged commit 0bca925 into hashie:master May 5, 2020

dblock mentioned this pull request May 5, 2020

Integer key upsets Hashie::Mash with SymbolizeKeys extension #516

Closed

carolineartz deleted the fix-symbolize-keys branch May 8, 2020 17:48

michaelherold mentioned this pull request Nov 20, 2021

Behavior change in accessing Mash values in 5.0 release #556

Closed

jrochkind mentioned this pull request Jan 10, 2022

allow hashie 5.x traject/traject#281

Merged

michaelherold mentioned this pull request Apr 23, 2022

[Question] Why use :to_sym in Hashie::Mash#convert_key #563

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to `Mash` initialization key string conversion. #521

Changes to `Mash` initialization key string conversion. #521

carolineartz commented Apr 24, 2020 •

edited

This comment has been minimized.

dblock left a comment

carolineartz commented Apr 27, 2020

dblock commented Apr 27, 2020

This comment has been minimized.

dblock left a comment

carolineartz commented May 4, 2020

dblock commented May 4, 2020

dblock left a comment

dblock May 4, 2020

carolineartz May 4, 2020

dblock May 5, 2020

dblock May 5, 2020

BobbyMcWho May 5, 2020

dblock May 5, 2020

dblock commented May 5, 2020

Changes to Mash initialization key string conversion. #521

Changes to Mash initialization key string conversion. #521

Conversation

carolineartz commented Apr 24, 2020 • edited

Bug Fix

This comment has been minimized.

dblock left a comment

Choose a reason for hiding this comment

carolineartz commented Apr 27, 2020

dblock commented Apr 27, 2020

This comment has been minimized.

dblock left a comment

Choose a reason for hiding this comment

carolineartz commented May 4, 2020

dblock commented May 4, 2020

dblock left a comment

Choose a reason for hiding this comment

dblock May 4, 2020

Choose a reason for hiding this comment

carolineartz May 4, 2020

Choose a reason for hiding this comment

dblock May 5, 2020

Choose a reason for hiding this comment

dblock May 5, 2020

Choose a reason for hiding this comment

BobbyMcWho May 5, 2020

Choose a reason for hiding this comment

dblock May 5, 2020

Choose a reason for hiding this comment

dblock commented May 5, 2020

Changes to `Mash` initialization key string conversion. #521

Changes to `Mash` initialization key string conversion. #521

carolineartz commented Apr 24, 2020 •

edited