Deep merge fix #7
Conversation
`deep_merge` has a bug that prevents it replacing a true value with false in certain circumstances. Fixed.
spec/defines/file_spec.rb
Outdated
| @@ -340,7 +340,7 @@ def basic_structure(title, file: true) | |||
| owner: 'root', | |||
| group: 'root', | |||
| mode: '0644', | |||
| content: "---\nlist:\n- 1\n- 2\nhash:\n one: 1\n two: 2\nrepl:\n not_two: 7\n" | |||
| content: "---\nlist:\n- 1\n- 2\nhash:\n one: 3\n two: 2\nrepl:\n not_two: 2\n" | |||
There was a problem hiding this comment.
I think for better readability this whole is_expected test-block should be rewritten a bit. An expected output of not_two: 2 is a bit confusing.
I am guessing we can easily do that by switching around the order of the hashes (or renaming some keys)
There was a problem hiding this comment.
Yeah. This was a cobbled together functional test at 2am. I believe I've actually replicated everything it covers with proper tests, so now's a good time to remove it.
What I should to though is add a test for the specific issue in this PR.
Hmm. I think that's more of an alternate feature - the behaviour in this patch is what's described in the documentation. I'll implement it if there's a request for it, but otherwise I think this is good. |
Pull Request (PR) description
The
deep_mergegem has a longstanding issue where using thedeep_mergemethod will never replace atruevalue withfalse. Thedeep_merge!method does have the expected behaviour, and the code in this module already creates a duplicate object for the destination object, so the fix is simple.This Pull Request (PR) fixes the following issues
n/a