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

Avoid defining Hash#deep_merge and #deep_merge! #342

Merged

Conversation

jonathanhefner
Copy link
Contributor

@jonathanhefner jonathanhefner commented Oct 2, 2023

config uses DeepMerge.deep_merge! instead of Hash#deep_merge!, so monkey patching Hash is unnecessary. Furthermore, DeepMerge's Hash monkey patch is not compatible with Rails 7.1 (see rails#49457).

This commit changes require 'deep_merge' to require 'deep_merge/core' so that DeepMerge's Hash monkey patch is no longer loaded. Users who rely on the monkey patch can load it manually via require 'deep_merge/deep_merge_hash'.

Closes #314.

@pkuczynski
Copy link
Member

@cjlarose what do you think?

Copy link
Member

@pkuczynski pkuczynski left a comment

Choose a reason for hiding this comment

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

Could you please update the CHANGELOG as well?

`config` uses `DeepMerge.deep_merge!` instead of `Hash#deep_merge!`, so
monkey patching `Hash` is unnecessary.  Furthermore, DeepMerge's `Hash`
monkey patch is not compatible with Rails 7.1 (see [rails#49457][]).

This commit changes `require 'deep_merge'` to `require 'deep_merge/core'`
so that DeepMerge's `Hash` monkey patch is no longer loaded.  Users who
rely on the monkey patch can load it manually via
`require 'deep_merge/deep_merge_hash'`.

Closes rubyconfig#314.

[rails#49457]: rails/rails#49457
@jonathanhefner jonathanhefner force-pushed the deep_merge-require-core-only branch from 6f335b1 to 4a0b7a5 Compare October 3, 2023 17:28
@jonathanhefner
Copy link
Contributor Author

Could you please update the CHANGELOG as well?

Done!

@pkuczynski
Copy link
Member

@jonathanhefner I extended our test suite for Rails 7.x in #344, but test fails with a can't modify frozen Array error. We need this to verify your proposed solution. As I am not developing in Ruby anymore for several years, I am not able to fix it quickly. Would you mind having a look at #344?

@pkuczynski
Copy link
Member

#344 got finally merged so we can verify this PR agains Rails 7.x. Let's see if it works.

@pkuczynski
Copy link
Member

Looks it works! Merging and releasing...

@pkuczynski pkuczynski merged commit 759c0fe into rubyconfig:master Oct 18, 2023
7 checks passed
tvararu added a commit to nhsuk/manage-vaccinations-in-schools that referenced this pull request Oct 19, 2023
config@5.0.0 is compatible with Rails 7.1 rubyconfig/config#342
tvararu added a commit to nhsuk/manage-vaccinations-in-schools that referenced this pull request Oct 19, 2023
config@5.0.0 is compatible with Rails 7.1 rubyconfig/config#342
tvararu added a commit to nhsuk/manage-vaccinations-in-schools that referenced this pull request Oct 19, 2023
config@5.0.0 is compatible with Rails 7.1 rubyconfig/config#342
@pkuczynski pkuczynski added this to the 5.0.0 milestone Dec 18, 2023
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.

avoid Hash monkey patch when loading DeepMerge
2 participants