Skip merging if the destination is nil

[dtaniwaki]

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 } } because nil is set purposely. If you want to have { bar: 1 }, we can merge { foo: {} }.

What do you think?

[danielsdeleo]

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.

[pkuczynski]

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.

[danielsdeleo]

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).

[pkuczynski]

Sure, we could do it in this way. Shall I prepare upgraded PR which introduce new option to turn this behaviour on?

[danielsdeleo]

that would be most welcome