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

Symbolize names and freeze values when loading from JSON #587

Merged
merged 1 commit into from Jan 25, 2022

Conversation

paarthmadan
Copy link
Contributor

Following a comment in #583, this PR uses new options in JSON to freeze and symbolize names at parse time.

I can append this commit to the other PR if that'd make it easier, but I opted to keep PRs as granular as possible for ease of review.

@@ -253,7 +253,7 @@ def load_yml(filename)
# toplevel keys.
def load_json(filename)
begin
::JSON.parse(File.read(filename))
::JSON.parse(File.read(filename), symbolize_names: true, freeze: true)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find the freeze option mentioned in the docs. So I tried it out:

[10] pry(main)> obj = JSON.parse(%Q{{ "one": "two" }}, freeze: true)
=> {"one"=>"two"}
[11] pry(main)> obj["one"].frozen?
=> false

It seems like it might not be a supported option. Unsupported options appear to be ignored. Here's an example of me using my name as an option:

obj = JSON.parse(%Q{{ "one": "two" }}, ryan: true)
=> {"one"=>"two"}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because you JSON is too old:

>> JSON.parse('"foo"', freeze: true).frozen?
=> true
>> JSON::VERSION
=> "2.5.1"

Which is fine, the feature is best effort.

@@ -253,7 +253,7 @@ def load_yml(filename)
# toplevel keys.
def load_json(filename)
begin
::JSON.parse(File.read(filename))
::JSON.parse(File.read(filename), symbolize_names: true, freeze: true)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paarthmadan you could do the same than for Psych, check for JSON.respond_to?(:load_file), it's a method that was added recently, a bit after freeze, and it will also give you bootsnap caching https://github.com/Shopify/bootsnap/blob/master/CHANGELOG.md#190

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, added 👍

JSON.load_file being defined implies that symbolize_names and freeze
options are supported at parse time. We use this as a best effort
feature test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants