Add functionality for overriding dictionary merges. #61

Rtzq0 · 2017-01-02T10:36:06Z

Dictionary merges can now be overridden at any level by usage
of a special character. The tilde is chosen as base default.

This PR is a resolution to #60

lottspot

I haven't had a chance to actually test this change out yet, but a couple of overarching things from my read through of the patch:

I'm trying to understand the role of the initmerge parameter, but probably need help getting it
We should probably provide some way for the user to escape the special meaning of PARAMETER_DICT_KEY_OVERRIDE_PREFIX

Overall though, I think this is a great feature being done with the right implementation strategy

lottspot · 2017-01-02T17:01:00Z

reclass/datatypes/parameters.py

+            cur (dict): Current dictionary
+            new (dict): Dictionary to be merged
+            path (string): Merging path from recursion
+            initmerge (bool): True if called as part of entity init


What is an example of a case in which a Parameters object performs a merge as part of Entity initialization? I don't see any merges that occur in Entity.__init__

The merge is actually performed by Parameters.__init__ which happens to be called by Entity.__init__. Lines 47-50 of Parameters.__init__...

if mapping is not None: # we initialise by merging, otherwise the list of references might # not be updated self.merge(mapping)

In actuality this happens, as far as I can tell, every time.

Ahh, ok make total sense. The note could be a bit less confusing if edited to state "True if called as part of Parameters init" rather than "entity init".

lottspot · 2017-01-02T17:06:56Z

reclass/datatypes/parameters.py

-                                           path.new_subpath(key))
+            if key.startswith(ovrprfx) and not initmerge:
+                ret[key.lstrip(ovrprfx)] = newvalue
+            else:


I think that the case if key.startswith(overprfx) and initmerge should be explicitly handled. I don't think we want to allow processed parameter keys to contain special characters without those characters being explicitly escaped.

I agree with this in principle but in practice it's a mite hairier... in reality processed parameter keys never actually reach the user because merge is called outside init in core. I've tested this by creating a node with no classes and "tilde-params". Furthermore if we strip them in the __init__ merge they do not exist as a signalling character during the "proper" merge, in which case it merges as normal (contrary to expectations).

I'm not averse to reformulating this conditional but I'd maybe like to see something that you think would work. I futzed with this a lot and there's some delicate interplay going between Parameters.__init__ and Core._nodeinfo

Hmm... I think I see what you're saying here. This certainly does present a practical implementation challenge. I think I need to get my hands dirty a bit with this feature to see how it all plays out.

lottspot · 2017-01-02T17:07:51Z

reclass/datatypes/tests/test_parameters.py

@@ -188,6 +188,18 @@ def test_merge_dicts_overwrite(self):
        goal.update(mergee)
        self.assertDictEqual(p.as_dict(), dict(dict=goal))

+    def test_merge_dicts_override(self):


hm yes unit tests.... A Very Good Idea

I was WAAAAY too tired when I wrote those. Replicated the functionality of other tests AND missed validating that we override properly on list extensions as well!

Much better and cleaner now.

Rtzq0 · 2017-01-02T18:08:19Z

RE 1. I'm trying to understand the role of the initmerge parameter, but probably need help getting it:

The only time a Parameters() instance is merged with a dict is during __init__. At every other time it's merged with a Parameters() instance. initmerge is just a signalling argument that indicates that we've detected that this is an __init__ merge based on the fact that we're being merged with a dict object.

RE 2. We should probably provide some way for the user to escape the special meaning of PARAMETER_DICT_KEY_OVERRIDE_PREFIX

I'd like to open a separate non-blocking issue for this, the reason being that it'll be more consistent for users if the "special behavior character" is consistent between this and the applications negations (which are currently hardcoded with no escaping) and for the escaping system to also be consistent between those sections.

Dictionary merges can now be overridden at any level by usage of a special character. The tilde is chosen as base default.

lottspot · 2017-01-02T18:49:34Z

The only time a Parameters() instance is merged with a dict is during init. At every other time it's merged with a Parameters() instance. initmerge is just a signalling argument that indicates that we've detected that this is an init merge based on the fact that we're being merged with a dict object.

Makes total sense now, thanks! I might suggest revising this approach to go one of two ways though:

Fully explicit (probably better): Add the init parameter to Parameters.merge directly and set it explicitly within Parameters.__init__. Don't try to guess whether it is an init merge at all.
Fully implicit (probably less good): Don't add it as a parameter anywhere. Guess whether it is an initial merge based on the (lack of) contents of self._base.

I think going at it one of these two ways will make the intent less confusing to read and would make the code itself more robust to changes

I'd like to open a separate non-blocking issue for this, the reason being that it'll be more consistent for users if the "special behavior character" is consistent between this and the applications negations (which are currently hardcoded with no escaping) and for the escaping system to also be consistent between those sections.

I certainly agree-- We have this problem elsewhere in the codebase right now, and we should solve it in a unified way, which probably requires its own dedicated effort. You're probably right in that it shouldn't block this merge, but since this merge does represent a breaking change with user data formats, and until that problem is solved the breakage will have no remediation, it's definitely a problem we'll need to address before another stable release

Rtzq0 · 2017-01-03T01:35:17Z

I have pushed another commit that takes your preferred approach to the initmerge argument. I did not initially do it that way intentionally but I have been convinced that explicit is probably better.

I did not rebase this one because I thought it would be good to keep the extra reversion point, JIC.

lottspot · 2017-01-03T15:39:32Z

Kickass job on implementing this feature! I'd say it's ready for a merge and some real-world testing

bbinet · 2017-04-05T13:25:41Z

It would be great if the same feature could also be implemented for the lists: overwriting a list instead of extending when the special ~ character is used.

Rtzq0 · 2017-04-05T14:15:45Z

@bbinet that is definitely how I /wanted/ to do it, but I hit some surprising complexity. If this represents a use case/need for you, please open and issue so that I have something to track it so I don't forget to work on this problem :-)

bbinet · 2017-04-05T14:32:16Z

@Rtzq0 I've just created #70
I'm curious about the surprising complexity, but let's discuss it in the #70 issue.

bbinet · 2017-05-03T15:55:38Z

@Rtzq0 I meant #75

Bump 156

Rtzq0 added enhancement in-progress planned labels Jan 2, 2017

Rtzq0 self-assigned this Jan 2, 2017

Rtzq0 requested a review from lottspot January 2, 2017 10:36

lottspot reviewed Jan 2, 2017

View reviewed changes

Add functionality for overriding dictionary merges.

99acc6e

Dictionary merges can now be overridden at any level by usage of a special character. The tilde is chosen as base default.

Rtzq0 force-pushed the issue_60 branch from a247e94 to 99acc6e Compare January 2, 2017 18:30

made initmerge explicit

b388533

Rtzq0 mentioned this pull request Jan 3, 2017

Make 'special behavior characters' configurable and escapable #63

Open

Rtzq0 added this to the 1.4.2 milestone Jan 3, 2017

lottspot merged commit 75dbf1e into madduck:master Jan 3, 2017

Rtzq0 deleted the issue_60 branch January 3, 2017 19:25

bbinet mentioned this pull request Apr 5, 2017

Add functionality for overriding lists instead of extending #75

Open

AndrewPickford pushed a commit to AndrewPickford/reclass that referenced this pull request Sep 11, 2018

Merge pull request madduck#61 from salt-formulas/bump-156

7fb396d

Bump 156

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functionality for overriding dictionary merges. #61

Add functionality for overriding dictionary merges. #61

Rtzq0 commented Jan 2, 2017

lottspot left a comment

lottspot Jan 2, 2017

Rtzq0 Jan 2, 2017 •

edited

Loading

lottspot Jan 2, 2017

lottspot Jan 2, 2017

Rtzq0 Jan 2, 2017 •

edited

Loading

lottspot Jan 2, 2017

lottspot Jan 2, 2017

Rtzq0 Jan 2, 2017

Rtzq0 commented Jan 2, 2017

lottspot commented Jan 2, 2017

Rtzq0 commented Jan 3, 2017

lottspot commented Jan 3, 2017

bbinet commented Apr 5, 2017

Rtzq0 commented Apr 5, 2017

bbinet commented Apr 5, 2017

bbinet commented May 3, 2017

Add functionality for overriding dictionary merges. #61

Add functionality for overriding dictionary merges. #61

Conversation

Rtzq0 commented Jan 2, 2017

lottspot left a comment

Choose a reason for hiding this comment

lottspot Jan 2, 2017

Choose a reason for hiding this comment

Rtzq0 Jan 2, 2017 • edited Loading

Choose a reason for hiding this comment

lottspot Jan 2, 2017

Choose a reason for hiding this comment

lottspot Jan 2, 2017

Choose a reason for hiding this comment

Rtzq0 Jan 2, 2017 • edited Loading

Choose a reason for hiding this comment

lottspot Jan 2, 2017

Choose a reason for hiding this comment

lottspot Jan 2, 2017

Choose a reason for hiding this comment

Rtzq0 Jan 2, 2017

Choose a reason for hiding this comment

Rtzq0 commented Jan 2, 2017

lottspot commented Jan 2, 2017

Rtzq0 commented Jan 3, 2017

lottspot commented Jan 3, 2017

bbinet commented Apr 5, 2017

Rtzq0 commented Apr 5, 2017

bbinet commented Apr 5, 2017

bbinet commented May 3, 2017

Rtzq0 Jan 2, 2017 •

edited

Loading

Rtzq0 Jan 2, 2017 •

edited

Loading