[RFR] Merge data structures #88

rsnyman · 2018-09-27T16:47:42Z

Add the ability to merge nested structures instead of completely overwriting them

Please let me know if this looks like an acceptable solution, and what I should change to make it better.

rochacbruno · 2018-09-27T18:09:15Z

dynaconf/utils/__init__.py

@@ -25,6 +25,21 @@ def dictmerge(old, new):
    return new


+def deep_merge(old, new):


There is already this function https://github.com/rochacbruno/dynaconf/blob/master/dynaconf/utils/__init__.py#L15 dictmerge which is used by dynaconf cli write commands. I think the purposes are the same. Maybe we can keep only one function for this and extend its functionality. (you implementation looks more complete)

Yes, I was thinking of doing that. I tried dictmerge initially, but it only handles dicts. I'll replace dictmerge with my function.

it should be clearly defined if this should be a deep update or a merged copy, one shouldn't have a return value, the other shouldn't affect its arguments

@RonnyPfannschmidt good point! in_place argument can control it. @rsnyman

i would strongly advise towards picking one implementation behavior and then sticking to it
i have a gut feeling that choice in this dimension is going to be more expensive than desirable

@RonnyPfannschmidt Good point. An in-place merge is probably better.

rochacbruno · 2018-09-27T18:16:35Z

dynaconf/default_settings.py

@@ -85,6 +85,9 @@ def start_dotenv(obj=None):
 # The default encoding to open settings files
 ENCODING_FOR_DYNACONF = get('ENCODING_FOR_DYNACONF', 'utf-8')

+# Merge dictionaries on load
+MERGE_CONF_FOR_DYNACONF = get('MERGE_CONF_FOR_DYNACONF', False)


I like having this feature as optional, and this config enables it globally, what about cases where we need only specific data to be merged and others to be overwritten.

Is it a good idea to have a kind of super() keyword (e.g: _merge, or dynaconf_merge) to give a fine control over where the merge is being applied.

Example.
settings.yml

default: data: one: 1 two: 2

Now if I want to add a three I can

othersettings.yaml

default: _dynaconf_merge: true data: three: 3

or maybe

yeatanothersettings.yaml

default: data: _dynaconf_merge: true four: 4

and at the end I have

>> settings.DATA { 'one': 1, 'two': 2, 'three': 3, 'four': 4, }

The same can be applied to environment variables

$ export DYNACONF_DATA="{five = 5, six = 6, dynaconf_merge = true}"

If I dont supply the special key dynaconf_merge (we can name it just _merge), then the cascade will overwrite the values.

What do you think is best?

Fine control

global set

I like the idea of fine control, but I'm not sure that another key in the settings file is necessarily the correct way to do it. It looks like a setting here, but when you actually look at your settings, there's no key.
Perhaps a better approach would be to start with a global setting, and then think about a good way to implement the fine control.

@rsnyman agreed! the fine control doesn't block the global implementation.

If we are going to add special keywords then we need to think about the naming and test if all the file formats supports names like _dynaconf_merge, _dynaconf_include etc.

rochacbruno · 2018-09-27T18:25:23Z

When #84 is implemented, we can also support export DYNACONF_DATA="inner.deeper=1" and then have {"data": {"inner": {"existing_key": "existing_value", "deeper": 1}}}

RonnyPfannschmidt · 2018-09-27T19:22:39Z

dynaconf/utils/__init__.py

+    """
+    if isinstance(old, (tuple, list)):
+        return old + new
+    if isinstance(old, dict) and isinstance(new, dict):


do we want to keep track of where a value is from (imagine wanting to write current updates/changes to a configfile while ignoring all inherited data)

codecov · 2018-09-28T22:16:44Z

Codecov Report

Merging #88 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #88   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          17     17           
  Lines         568    570    +2     
=====================================
+ Hits          568    570    +2

Impacted Files	Coverage Δ
dynaconf/loaders/json_loader.py	`100% <100%> (ø)`	⬆️
dynaconf/loaders/toml_loader.py	`100% <100%> (ø)`	⬆️
dynaconf/loaders/py_loader.py	`100% <100%> (ø)`	⬆️
dynaconf/loaders/yaml_loader.py	`100% <100%> (ø)`	⬆️
dynaconf/loaders/ini_loader.py	`100% <100%> (ø)`	⬆️
dynaconf/utils/__init__.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61c3618...458ffa6. Read the comment docs.

rsnyman · 2018-10-04T16:58:10Z

@rochacbruno Is there anything else you want me to do with this?

rochacbruno · 2018-10-04T18:48:13Z

dynaconf/base.py

@@ -494,10 +494,17 @@ def set(self, key, value, loader_identifier=None, tomlfy=False):
        :param tomlfy: Bool define if value is parsed by toml (defaults False)
        """
        value = parse_conf_data(value, tomlfy=tomlfy)
+        key = key.strip().upper()
+
+        if 'FOR_DYNACONF' not in key and getattr(self,


I am not sure I understand this condition, can you explain the case where FOR_DYNACONF key is set and should not be merged.

Also about the name of the variable MERGE_CONF_FOR_DYNACONF should we adopt something different?

ENABLE_MERGE_FOR_DYNACONF MERGE_ENABLED_FOR_DYNACONF

?

I was experiencing some issues with keys, I'll test it again and see if it makes a difference.

As for the name, I don't mind either of those, I would just recommend they follow the same convention as the rest of the names.

If I remove the check for DynaConf variables, I get the following traceback:

Traceback (most recent call last): ... File "dynaconf/base.py", line 500, in set attr = self.get(key, default=None) File "dynaconf/base.py", line 231, in get (fresh or self._fresh or key in self.FRESH_VARS_FOR_DYNACONF) and AttributeError: 'Settings' object has no attribute 'FRESH_VARS_FOR_DYNACONF'

Having said that, if I change line 500 in base.py to use getattr instead, it works.

attr = getattr(self, key, None)

rochacbruno · 2018-10-08T15:53:02Z

@rsnyman I' ll perform some manual tests on this PR this week .

sirex

I just tested it manually with settings.toml:

[default]
foo.bar = 1
foo.baz = 2

[testing]
foo.bar = 3

And it works:

from dynaconf import LazySettings

settings = LazySettings(MERGE_ENABLED_FOR_DYNACONF=True)

settings.setenv('testing')
print(settings.FOO)

Output:

{'bar': 3, 'baz': 2}

sirex · 2018-10-10T11:19:28Z

tests/test_base.py

+    settings.set('MERGE_KEY', [{'name': 'item 1'}, {'name': 'item 2'}])
+    settings.set('MERGE_KEY', [{'name': 'item 3'}, {'name': 'item 4'}])
+    assert settings.MERGE_KEY == [{'name': 'item 1'}, {'name': 'item 2'},
+                                  {'name': 'item 3'}, {'name': 'item 4'}]


It looks, that this only tests list case, but not nested dicts. Probably it would be useful to also tests nested dicts.

- Addd the ability to merge nested structures instead of completely overwriting them - Use monkeypatch to stop one test from interferring with another - Updated documentation

rochacbruno · 2018-10-19T16:17:52Z

Merged and added examples https://github.com/rochacbruno/dynaconf/tree/master/example/merge_configs

- Added `MERGE_ENABLED_FOR_DYNACONF` with ability to merge nested dictionaries instead of replacing PR #88 - Support for dot notation to access nested dictionaries like `settings['KEY.OTHER.NESTED.DEEPER']` PR #93 - Support dotted notation for validators PR #98 - Fixed a bug in SETTINGS_MODULE cleanup when `.setenv` method was called PR #97 - Added Python 3.7 to test matrix PR #99

rochacbruno reviewed Sep 27, 2018

View reviewed changes

RonnyPfannschmidt reviewed Sep 27, 2018

View reviewed changes

rsnyman force-pushed the merge-settings branch 3 times, most recently from 835402b to d7214c4 Compare September 28, 2018 22:01

rsnyman changed the title ~~[WIP] Merge data structures~~ [RFR] Merge data structures Oct 2, 2018

rochacbruno reviewed Oct 4, 2018

View reviewed changes

rsnyman force-pushed the merge-settings branch from d7214c4 to e2b3dd3 Compare October 4, 2018 21:46

sirex approved these changes Oct 10, 2018

View reviewed changes

Add setting merging

458ffa6

- Addd the ability to merge nested structures instead of completely overwriting them - Use monkeypatch to stop one test from interferring with another - Updated documentation

rsnyman force-pushed the merge-settings branch from e2b3dd3 to 458ffa6 Compare October 10, 2018 23:05

rochacbruno merged commit 458ffa6 into dynaconf:master Oct 19, 2018

rochacbruno mentioned this pull request Oct 19, 2018

RFC: Allow dot notation for .set method #96

Closed

6 tasks

jperras mentioned this pull request Oct 31, 2018

Allow dotted-path based setting of configuration key/value pairs. #104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFR] Merge data structures #88

[RFR] Merge data structures #88

rsnyman commented Sep 27, 2018

rochacbruno Sep 27, 2018

rsnyman Sep 27, 2018

RonnyPfannschmidt Sep 27, 2018

rochacbruno Sep 27, 2018

RonnyPfannschmidt Sep 27, 2018

rsnyman Sep 27, 2018

rochacbruno Sep 27, 2018 •

edited

rsnyman Sep 27, 2018

rochacbruno Sep 27, 2018

rochacbruno commented Sep 27, 2018

RonnyPfannschmidt Sep 27, 2018

codecov bot commented Sep 28, 2018 •

edited

rsnyman commented Oct 4, 2018

rochacbruno Oct 4, 2018

rsnyman Oct 4, 2018

rsnyman Oct 4, 2018

rsnyman Oct 4, 2018

rochacbruno commented Oct 8, 2018

sirex left a comment

sirex Oct 10, 2018

rochacbruno commented Oct 19, 2018

		@@ -25,6 +25,21 @@ def dictmerge(old, new):
		return new


		def deep_merge(old, new):

[RFR] Merge data structures #88

[RFR] Merge data structures #88

Conversation

rsnyman commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochacbruno Sep 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochacbruno commented Sep 27, 2018

Choose a reason for hiding this comment

codecov bot commented Sep 28, 2018 • edited

Codecov Report

rsnyman commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochacbruno commented Oct 8, 2018

sirex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochacbruno commented Oct 19, 2018

rochacbruno Sep 27, 2018 •

edited

codecov bot commented Sep 28, 2018 •

edited