Merge nil values or keep the original via an option

[jrafanie]

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 #20 and can be confusing to see the difference.
This commit handles merging a nil value into an existing destination via
an option.
PR #20 handles NOT merging a value into an already nil destination.

[Fryguy]

This change looks good to me. Not sure on the option name nor description as they don't seem to describe properly what this does, but we can debate that. @jrafanie, I believe you are ultimately trying to get this into railsconfig/config, so I would like some input from @pkuczynski here as well. @pkuczynski thoughts? Here is some WIP code from jrafanie on how it might work in the config gem: jrafanie/config@b8af322

See also rubyconfig/config#13

[pkuczynski]

@Fryguy I think the name of the option is not the best choice, although thinking about it for a while, I am not sure how could it be named differently (prosed default value maintains current behavior)? Any other suggestions?

• override_with_nil (default: false)
• nil_overrides (false)
• skip_nil (true)
• skip_nill_values (true)
• discard_nil_values (true)

This PR is also missing documentation update (README.md).

Other than that it is indeed required feature for config gem (I made a mistake saying that #20 solves the problem) and I would welcome it very much!

@jrafanie any chance to make updates? @danielsdeleo any chance to get that merged?

[jrafanie]

Yes @pkuczynski I can make those changes.

From your suggestions, I think I like skip_nil_values (true) the most although naming is hard and I'll change my mind tomorrow. I think that describes more what a consumer of this method would expect the interface to look like:

hash_src = {"item" => nil}
hash_dst = {"item" => 'existing'}
DeepMerge::deep_merge!(hash_src, hash_dst, {:skip_nil_values => false})

assert_equal({"item" => nil}, hash_dst)

If you agree, I'll update the PR and add some doc. Thanks for the review.

[pkuczynski]

Yeah, naming always puzzles my head too :)

I think the best name for an option is when it is human readable. So in my mind, I would rather expect that non-default some_option_name = true would turn on an additional feature. That's why I am in favor of positive names like nil_should_override, instead of skip, discard, which sound like double negation.

But in this case it is very hard to come up with a name for me too, so I guess we can proceed with skip_nil_values

[pkuczynski]

After reading what I wrote above, I think skip_nil_values is the best option, unless someone thinks otherwise?

[jrafanie]

@pkuczynski I agree on usually preferring the positive form instead of double negation...

the only positive I can come up with is:
DeepMerge::deep_merge!(hash_src, hash_dst, {:merge_nil_values => true})

Again, naming...

[pkuczynski]

:merge_nil_values sounds good...

[jrafanie]

Ok, @pkuczynski updated

[jrafanie]

Updated the WIP my railsconfig branch to show how it would be used there: jrafanie/config@fe672d2

[Fryguy]

LGTM

[Fryguy]

@jrafanie I just realized there's another section in the README that needs an update...can you open another PR? See https://github.com/danielsdeleo/deep_merge#options

[jrafanie]

Thanks @pkuczynski @Fryguy for the fast review and merge!

Once #33 is merged, a tag is created and pushed to rubygems.org, I can open the PR on on the config gem that will look something like this. We can discuss there if this change in behavior should be optional via an opt-in or opt-out.

Does that sound good @pkuczynski @Fryguy?

[pkuczynski]

Sounds god to me! I won't be able to do any work on config gem for the next two weeks, but please create PR after deep_merge is released and I will review it as soon as I can...

Thanks for your contribution @jrafanie!