-
-
Notifications
You must be signed in to change notification settings - Fork 230
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 overwrite_arrays option #137
Conversation
This will fail Travis until danielsdeleo/deep_merge#21 is merged and the gemspec is updated here. |
The ability to override Arrays has been made into a PR upstream at rubyconfig/config#137 and danielsdeleo/deep_merge#21 . However, they are not yet merged. When they are merged and released, we can switch back to a released version. Supercedes #7656 Paired on this with @carbonin
@pkuczynski @fredwu Not sure who to ping on this, so hoping I pick the right people 🙏 Do you also do reviews / merge on https://github.com/danielsdeleo/deep_merge ? |
@Fryguy No I don't believe any of us do - in fact by the looks of it |
The ability to override Arrays has been made into a PR upstream at rubyconfig/config#137 and danielsdeleo/deep_merge#21 . However, they are not yet merged. When they are merged and released, we can switch back to a released version. Supercedes ManageIQ#7656 Paired on this with @carbonin
I've updated my PR on deep_merge...hopefully it will pass now...then I'll see if I can get a new deep_merge version released |
@Fryguy it seems that you have some conflicts. Could you please rebase on top of current master and squash your commits into one? I got a message from @danielsdeleo and he will look at the deep_merge PR in June... |
My deep_merge PR was merged...just need a release of deep_merge itself. |
When do you think it will be released? |
+1 |
@ALL I submitted a new pull request where I pushed the same gem under a new name (deep_merge2). The latest release of the deep_merge gem was in 2011, so I guess it wasn't going to be released soon. link to pull request: #147 Note: I rebased the branch onto the master of railsconfig/config. It should be mergeable. :) |
@pkuczynski I'll rebase this today...I'll also see if I can @danielsdeleo to release a new deep_merge |
@pkuczynski I just got release rights on the deep_merge gem. Expect a new version soon and then I will update this PR with the new gem. |
This is an extraction from rubyconfig#103 where @dtaniwaki had added this option, but had done it prior to the deep_merge extraction. This depends on danielsdeleo/deep_merge#21 Paired with @carbonin <https://github.com/carbonin> on this.
23ce4b8
to
a0eb749
Compare
@pkuczynski I released a new version of the deep_merge gem and updated this PR accordingly. |
Cool stuff. Any objections before merging? |
@pkuczynski @fredwu Bump? Incidentally, we've been using this branch in ManageIQ / CloudForms for quite a while now, and all seems well! |
I am on holiday at the moment. Can have a look when I am back |
@pkuczynski Sounds great! Enjoy your holiday! 😎 |
The only thing I will change is to make it default to 'true', as this should be the correct behaviour from the start I believe. I will also add a note in the documentation, to highlight this change. |
🎉 Thanks @pkuczynski |
This is an extraction from
#103 where @dtaniwaki had
added this option, but had done it prior to the deep_merge extraction.
This depends on danielsdeleo/deep_merge#21
Paired with @carbonin https://github.com/carbonin on this.