Avoid activesupport deep_merge conflict #213
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-changelog-generator depends on activesupport which defines `Hash#deep_merge` methods that behave differently and conflict with the ones from the `deep_merge` gem. Use the recommended way of avoiding this conflict, as per the `deep_merge` documentation.[1] Also, revert the changes from voxpupuli#209 which cause unexpected behavior with generated fixtures. [1] https://github.com/danielsdeleo/deep_merge#using-deep_merge-in-rails
ekohl
approved these changes
May 14, 2021
| @@ -22,7 +22,7 @@ class Generator | |||
| def generate(layout, options) | |||
| layout = prepare(layout) | |||
| tokens = tokenize_layout(layout) | |||
| config = {}.deep_merge(BASE_CONFIG) | |||
| config = {}.deeper_merge(BASE_CONFIG) | |||
There was a problem hiding this comment.
Should this be just
Suggested change
| config = {}.deeper_merge(BASE_CONFIG) | |
| config = BASE_CONFIG.dup |
Or does deeper_merge also duplicate CONFIG within BASE_CONFIG?
Taking a deeper look: what is consoleport? In none of the beaker projects I can find any documentation. Is it something that only one provider (abs?) needs? Safe for nfs_server.
There was a problem hiding this comment.
Honestly I have no idea... consoleport sounds like it could be a PE-specific thing, but I
don't know about nfs_server. I don't see them used anywhere in our projects... maybe it would make sense to remove both in a major release.
Or does
deeper_mergealso duplicateCONFIGwithinBASE_CONFIG?
That seems to be the case, object IDs are different with deeper_merge:
[20] pry(#<BeakerHostGenerator::Generator>)> BASE_CONFIG.dup["CONFIG"].object_id == BASE_CONFIG["CONFIG"].object_id
=> true
[21] pry(#<BeakerHostGenerator::Generator>)> {}.deeper_merge(BASE_CONFIG)["CONFIG"].object_id == BASE_CONFIG["CONFIG"].object_id
=> false
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
github-changelog-generator depends on activesupport which defines
Hash#deep_mergemethods that behave differently and conflict with the ones from thedeep_mergegem. Use the recommended way of avoiding this conflict, as per thedeep_mergedocumentation.[1]Also, revert the changes from #209 which cause unexpected behavior with generated fixtures.
[1] https://github.com/danielsdeleo/deep_merge#using-deep_merge-in-rails