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

Add explicit implementation of Config::Options#as_json #292

Merged
merged 2 commits into from
Dec 8, 2020

Conversation

cjlarose
Copy link
Member

@cjlarose cjlarose commented Dec 1, 2020

Fixes #287

Before, Config::Options#as_json was not explicitly defined, but the class included the Enumerable module. So, if you were using ActiveSupport Core Extensions, then the definition of #as_json for Enumerable was invoked, which would represent the Config::Options as a Array of key-value pairs instead of a Hash.

This changes brings #as_json in line with the output we get from to_json: essentially, calling these methods on an instance of Config::Options behaves the same as if they were called instead on the hash returned by to_hash.

It's not clear if this is a "bug fix" or a "breaking change" from a semver perspective, but I think it's unlikely that someone was depending on the output from #as_json to be the Array-of-pairs representation.

Before, `Config::Options#as_json` was not explicitly defined, but the
class included the `Enumerable` module. So, if you were using
ActiveSupport Core Extensions, then the definition of `#as_json` for
Enumerable was invoked, which would represent the `Config::Options` as a
Array of key-value pairs instead of a Hash.

This changes brings `#as_json` in line with the output we get from
`to_json`: essentially, calling these methods on an instance of
`Config::Options` behaves the same as if they were called instead on
the hash returned by `to_hash`.
@cjlarose cjlarose force-pushed the add-config-options-as-json branch from cc2d87a to 5a76012 Compare December 1, 2020 03:20
@cjlarose
Copy link
Member Author

cjlarose commented Dec 1, 2020

Tests fail when using the Sinatra Gemfiles, which is more-or-less expected: if you don't have the ActiveSupport Core Extensions loaded, then Hash#as_json isn't defined.

I don't know yet what the right fix is. I'm considering skipping the test except when testing against Rails, since the method sort of has an implicit dependency on ActiveSupport. If a user isn't in the Rails ecosystem or otherwise using ActiveSupport, then I wouldn't expect them to call #as_json anyway.

It's possible to, at runtime, check to_hash.repsond_to?(:as_json) and do something differently if it doesn't respond to that method, but it's not 100% clear what we'd return in that case.

@cjlarose cjlarose force-pushed the add-config-options-as-json branch from f96ae0a to e58a4ee Compare December 2, 2020 20:06
@cjlarose
Copy link
Member Author

cjlarose commented Dec 2, 2020

@pkuczynski Updated to skip the test except when running against Rails. LMK what you think!

@pkuczynski pkuczynski added the bug label Dec 8, 2020
@pkuczynski pkuczynski added this to the 2.3.0 milestone Dec 8, 2020
@pkuczynski
Copy link
Member

Looks very good to me! I will merge it in and release new version :) Thanks for all your hard work on this @cjlarose!

@pkuczynski pkuczynski merged commit 8c894be into rubyconfig:master Dec 8, 2020
@pkuczynski pkuczynski modified the milestones: 2.3.0, 2.2.2 Dec 8, 2020
ippachi pushed a commit to ippachi/config-1 that referenced this pull request Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Unexpected JSON serialisation behaviour
2 participants