Skip merging if the destination is nil #20
Conversation
|
Sorry I took so long to reply. While I would agree with you if we were implementing this from scratch, in this case, I think we need to stay "bug compatible." There could be a lot of folks relying on this even if they don't know about it. So if we were to do this, it would have to be opt-in or we could only do it in a major release. |
|
I would vote for introducing this feature with an option to turn it off. My gem rely on deep merge and I have some bug reports on this kind of behaviour too. |
|
I think that since this project hasn't changed much recently and also has historically introduced new behaviors in an opt-in way, users can reasonably expect that upgrading won't change anything, and therefore it would be best to make this opt-in. If it's a hassle to opt-in to the behavior then we could change the default setting in a future major release (which would be a 1 LOC change). |
|
Sure, we could do it in this way. Shall I prepare upgraded PR which introduce new option to turn this behaviour on? |
|
that would be most welcome |
For some people, it's unexpected that explicitly merging in a nil will
not overwrite an existing value.
`DeepMerge::deep_merge({:a => nil}, {:a => 1})`
Currently, we retain the 1 value.
My expectation is we'd get a nil value.
Since changing this is a change in behavior, and possibly not desirable,
I'm exposing an option to opt-in to this new behavior.
Note, this is related to danielsdeleo#20 and can be confusing to see the difference.
This commit handles merging a nil value into an existing destination via
an option.
PR danielsdeleo#20 handles NOT merging a value into an already nil destination.
For some people, it's unexpected that explicitly merging in a nil will
not overwrite an existing value.
`DeepMerge::deep_merge({:a => nil}, {:a => 1})`
Currently, we retain the 1 value.
My expectation is we'd get a nil value.
Since changing this is a change in behavior, and possibly not desirable,
I'm exposing an option to opt-in to this new behavior.
Note, this is related to danielsdeleo#20 and can be confusing to see the difference.
This commit handles merging a nil value into an existing destination via
an option.
PR danielsdeleo#20 handles NOT merging a value into an already nil destination.
For some people, it's unexpected that explicitly merging in a nil will
not overwrite an existing value.
`DeepMerge::deep_merge({:a => nil}, {:a => 1})`
Currently, we retain the 1 value.
My expectation is we'd get a nil value.
Since changing this is a change in behavior, and possibly not desirable,
I'm exposing an option to opt-in to this new behavior.
Note, this is related to danielsdeleo#20 and can be confusing to see the difference.
This commit handles merging a nil value into an existing destination via
an option.
PR danielsdeleo#20 handles NOT merging a value into an already nil destination.
For some people, it's unexpected that explicitly merging in a nil will
not overwrite an existing value.
`DeepMerge::deep_merge({:a => nil}, {:a => 1})`
Currently, we retain the 1 value.
My expectation is we'd get a nil value.
Since changing this is a change in behavior, and possibly not desirable,
I'm exposing an option to opt-in to this new behavior.
Note, this is related to danielsdeleo#20 and can be confusing to see the difference.
This commit handles merging a nil value into an existing destination via
an option.
PR danielsdeleo#20 handles NOT merging a value into an already nil destination.
I think we should not overwrite value if you purposely set nil.
e.g.
Consider merging
{ foo: { bar: 1 } }and{ foo: { bar: nil } }.It should merge them
{ foo: { bar: nil } }instead of{ foo: { bar: 1 } }becausenilis set purposely.If you want to have
{ bar: 1 }, we can merge{ foo: {} }.What do you think?