Merge nil values by default

[jrafanie]

Previously, if you merged in {a: nil}, it would not overwrite an
existing value. In #13, this was found to be an undesirable default
behavior.

Now, we will merge nil values by default. You can retain the old
behavior via config.merge_nil_values = false in your Config initializer.

This required changes to deep merge, found in: danielsdeleo/deep_merge#33
This was released in deep_merge 1.2.0+.

Fixes #13

[jrafanie]

Note, this is a bit controversial because it's changing behavior. If needed, this could be optional, defaulted to false (to maintain current behavior) and you can enable it via Config.merge_nil_values similar to the overwrite_arrays option.

so, the question is:

1. should this be optional
2. if so, what should be the default

I can change the PR based on this feedback. Note, this is to fix #13. Whatever is decided, we can update that issue with the right way solve that issue.

[pkuczynski]

Looks good, thanks! We would need few additional things before this can be merged:

• option to set merge_nil_values in Config.setup, so users can decide about this behavior
• documentation in README
• entry in CHANGELOG
• fix codeclimate issues

@jrafanie do you think you can work on this?

[jrafanie]

option to set merge_nil_values in Config.setup, so users can decide about this behavior

sure @pkuczynski, what do you want the default to be? If we change behavior to default to merge_vil_values: true, we'll need to bump versioning to account for this. If we default to false, and expose it as an option, we won't break anyone depending on the current behavior.

[pkuczynski]

Make the default true. We will bump a version and publish a warning in changelog :)

[jrafanie]

I got it @pkuczynski. I'll push those changes.

[jrafanie]

I didn't fix this cop offense to match the style of the existing code and thereby not change style in this feature.

[pkuczynski]

👍

[jrafanie]

I retained the format of the rest of the file here and didn't fix the line length cop.

[pkuczynski]

👍

[jrafanie]

@pkuczynski Please take a look. I have added your suggestions and fixed all "reasonable" cops without changing the existing style. See my comments above.

[pkuczynski]

Looks good! Thanks for all your work on this. I will release this as 1.7.0

[jrafanie]

Thanks @pkuczynski 👍

[pkuczynski]

Done: https://rubygems.org/gems/config/versions/1.7.0

[jrafanie]

@pkuczynski FYI, I started playing with this change more in depth in a rails app and it works! But, I did notice a minor change in behavior: we had multiple configurations sources with our "last" source coming from our monolith and defining a "ems" key that other sources would populate. Because the monolith specified the key but with no value, since those would be set by sub-sources, we were overriding the values with nil. I think that's the right behavior and we should just validate our yml so we don't accidentally squash values from other sources.

See: ManageIQ/manageiq#16982

[pkuczynski]

Well, that's the risk. Your monolith should not define null key or should come first. Other than that I am glad it worked and you like my gem :)

[jeremywadsack]

Just in case anyone else comes across this, this breaking changed tripped my up because it's not just explicit nil but implicit ones:

# settings.yml
section:
  area:
    setting: nil

# development.yml
section:
  area:
#    setting: your-email-here

This lead to an error undefined method 'setting' for nil:NilClass. We had used this pattern in the past to "document" settings or sections for settings. Or perhaps we were just lazy and left empty YAML "sections" defined which makes them nil instead of a hash.

The solution was to comment out (for documentation) or remove the sections that have no defined values:

# development.yml

# section:
#  area:
#    setting: your-email-here