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

Factor out deep_merge into AS::DeepMergeable #45411

Merged
merged 1 commit into from Sep 25, 2023

Conversation

jonathanhefner
Copy link
Member

The ActiveSupport::DeepMergeable module allows a class to provide deep_merge and deep_merge! methods simply by implementing a merge!(other, &block) method. Values will be deep merged only when they are compatible, according to deep_merge?. By default, that only includes instances of the same class or its subclasses. A class may override deep_merge? to further restrict or expand the domain of deep mergeable values.

This does introduce a small change in behavior. Previously, Hash#deep_merge would only deep merge Hash instances. Now, deep_merge will deep merge any DeepMergeable instances that are compatible with each other.


This was inspired by #45369 (comment). For now, I've marked ActiveSupport::DeepMergeable as :nodoc:, but I can remove that if we want to make it publicly available.

The `ActiveSupport::DeepMergeable` module allows a class to provide
`deep_merge` and `deep_merge!` methods simply by implementing a
`merge!(other, &block)` method.  Values will be deep merged only when
they are compatible, according to `deep_merge?`.  By default, that only
includes instances of the same class or its subclasses.  A class may
override `deep_merge?` to further restrict or expand the domain of deep
mergeable values.

This does introduce a small change in behavior.  Previously,
`Hash#deep_merge` would only deep merge `Hash` instances.  Now,
`deep_merge` will deep merge any `DeepMergeable` instances that are
compatible with each other.
@seanpdoyle
Copy link
Contributor

#45369 has been stale for some time now, so I've rebased it and pushed up the changes.

How else can I help to move these changes along as well?

@zzak zzak added this to the 7.1.0 milestone Sep 23, 2023
@rafaelfranca rafaelfranca merged commit 71613d3 into rails:main Sep 25, 2023
@morgoth
Copy link
Member

morgoth commented Sep 29, 2023

hi @jonathanhefner - since it's changing the current behavior, shouldn't it have a changelog entry?

@jonathanhefner
Copy link
Member Author

since it's changing the current behavior, shouldn't it have a changelog entry?

I'm not sure if that's necessary... Currently, ActiveSupport::DeepMergeable isn't a public API, and the only two classes that include it are Hash and ActionController::Parameters. So the only thing that has changed is something like { foo: params }.deep_merge({ foo: other_params }). (The value of :foo will now be params.deep_merge(other_params) instead of other_params.)

I think the changelog entry from #45369 might be sufficient. But I can add another entry if you feel it would be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants