Merge nil values by default #196

jrafanie · 2018-01-31T17:06:17Z

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 · 2018-01-31T22:42:43Z

lib/config/options.rb

@@ -127,6 +127,7 @@ def merge!(hash)
      current = to_hash
      DeepMerge.deep_merge!(hash.dup,
                            current,
+                            merge_nil_values: true,


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:

should this be optional

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 · 2018-02-02T09:47:59Z

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 · 2018-02-02T16:02:08Z

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 · 2018-02-02T18:03:12Z

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

jrafanie · 2018-02-07T20:44:54Z

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

jrafanie · 2018-02-07T22:47:27Z

lib/config.rb

  @@knockout_prefix = nil
+  @@merge_nil_values = true


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

jrafanie · 2018-02-07T22:47:57Z

CHANGELOG.md

@@ -2,6 +2,7 @@

 ## Unreleased

+* **WARNING:** nil values will now overwrite an existing value.  This change of behavior can be reverted via `config.merge_nil_values = false` in your Config initializer  ([#196](https://github.com/railsconfig/config/pull/196))


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

Previously, if you merged in {a: nil}, it would not overwrite an existing value. In rubyconfig#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 rubyconfig#13

jrafanie · 2018-02-07T22:54:46Z

@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 · 2018-02-08T11:23:35Z

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

jrafanie · 2018-02-08T16:04:13Z

Thanks @pkuczynski 👍

pkuczynski · 2018-02-09T15:53:54Z

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

jrafanie · 2018-02-09T17:45:08Z

@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 · 2018-02-10T10:13:57Z

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 · 2018-12-19T19:02:24Z

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

jrafanie force-pushed the use_discard_nil_value_to_keep_nils branch 2 times, most recently from d2eba65 to 50d3f80 Compare January 31, 2018 19:44

jrafanie commented Jan 31, 2018

View reviewed changes

jrafanie force-pushed the use_discard_nil_value_to_keep_nils branch from 0b2bbe1 to 73ad458 Compare February 7, 2018 22:10

jrafanie changed the title ~~Changes to work with deep_merge so we merge in nil values~~ Merge nil values by default Feb 7, 2018

jrafanie force-pushed the use_discard_nil_value_to_keep_nils branch 4 times, most recently from f1caeb1 to 56afa61 Compare February 7, 2018 22:45

jrafanie commented Feb 7, 2018

View reviewed changes

jrafanie force-pushed the use_discard_nil_value_to_keep_nils branch from 56afa61 to 3ccbc79 Compare February 7, 2018 22:49

jrafanie force-pushed the use_discard_nil_value_to_keep_nils branch from 3ccbc79 to 6d3b140 Compare February 7, 2018 22:52

pkuczynski added 6 commits February 8, 2018 12:05

Update CHANGELOG.md

45210f5

Update CHANGELOG.md

6e0c8f7

Update README.md

33dbc51

Update options.rb

3459670

Update options.rb

5efe114

Update config.rb

50a1856

pkuczynski approved these changes Feb 8, 2018

View reviewed changes

pkuczynski added 2 commits February 8, 2018 12:21

Merge branch 'master' into use_discard_nil_value_to_keep_nils

a6b2354

Update options.rb

d16a231

pkuczynski added this to the 1.6.2 milestone Feb 8, 2018

pkuczynski force-pushed the master branch from eec39ab to fe60905 Compare February 8, 2018 12:38

Merge branch 'master' into use_discard_nil_value_to_keep_nils

5389cf3

pkuczynski merged commit b75f4c6 into rubyconfig:master Feb 8, 2018

jrafanie deleted the use_discard_nil_value_to_keep_nils branch February 8, 2018 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge nil values by default #196

Merge nil values by default #196

jrafanie commented Jan 31, 2018 •

edited

Loading

jrafanie Jan 31, 2018

pkuczynski commented Feb 2, 2018

jrafanie commented Feb 2, 2018

pkuczynski commented Feb 2, 2018

jrafanie commented Feb 7, 2018

jrafanie Feb 7, 2018

pkuczynski Feb 8, 2018

jrafanie Feb 7, 2018

pkuczynski Feb 8, 2018

jrafanie commented Feb 7, 2018

pkuczynski commented Feb 8, 2018

jrafanie commented Feb 8, 2018

pkuczynski commented Feb 9, 2018

jrafanie commented Feb 9, 2018

pkuczynski commented Feb 10, 2018

jeremywadsack commented Dec 19, 2018

		@@ -2,6 +2,7 @@

		## Unreleased

		* WARNING: nil values will now overwrite an existing value. This change of behavior can be reverted via `config.merge_nil_values = false` in your Config initializer ([#196](https://github.com/railsconfig/config/pull/196))

Merge nil values by default #196

Merge nil values by default #196

Conversation

jrafanie commented Jan 31, 2018 • edited Loading

jrafanie Jan 31, 2018

Choose a reason for hiding this comment

pkuczynski commented Feb 2, 2018

jrafanie commented Feb 2, 2018

pkuczynski commented Feb 2, 2018

jrafanie commented Feb 7, 2018

jrafanie Feb 7, 2018

Choose a reason for hiding this comment

pkuczynski Feb 8, 2018

Choose a reason for hiding this comment

jrafanie Feb 7, 2018

Choose a reason for hiding this comment

pkuczynski Feb 8, 2018

Choose a reason for hiding this comment

jrafanie commented Feb 7, 2018

pkuczynski commented Feb 8, 2018

jrafanie commented Feb 8, 2018

pkuczynski commented Feb 9, 2018

jrafanie commented Feb 9, 2018

pkuczynski commented Feb 10, 2018

jeremywadsack commented Dec 19, 2018

jrafanie commented Jan 31, 2018 •

edited

Loading