Override mechanism for primitive value merge #76

malachib · 2024-08-25T09:25:04Z

Additional load parameter mergeoverride is a function-as-an-argument to customize a or b selection during primitive value type merges (such as ints, strings, etc)

I made this since I need something that can choose semver ranges in overlaid yaml build configs.

The "last overlay wins" approach is quite sensible, but once in a while you hit an edge case like this one.

semver comparisons quickly becoming a complicated set operation

zerwes · 2024-08-27T02:46:12Z

Hello @malachib
Thank you for your PR and enhancement.
And great - you included a test for the new feature. 👍 (btw: you are the first one to do this from the beginning)
Even if the scenario is somehow special, I can imagine that maybe others will benefit from it.
I just have some proposals for changes.

zerwes · 2024-08-27T02:59:10Z

setup.py

@@ -15,7 +15,8 @@
 installrequires = [
    'PyYAML<7',
    'Jinja2>3,<4',
-    'MarkupSafe<3'
+    'MarkupSafe<3',
+    'semantic-version>2'


As the dependency is not required for the basic functionality, I would like to take this out of the installrequires.

diff --git a/setup.py b/setup.py index 6360adb..1701d2c 100755 --- a/setup.py +++ b/setup.py @@ -15,8 +15,7 @@ long_description = open('README.rst').read() installrequires = [ 'PyYAML<7', 'Jinja2>3,<4', - 'MarkupSafe<3', - 'semantic-version>2' + 'MarkupSafe<3' ]

In order to make your test work, I would recommend using a new file

--- /dev/null 2024-08-25 06:33:22.535323361 +0200 +++ test/requirements.txt 2024-08-27 04:52:58.992351951 +0200 @@ -0,0 +1,2 @@ +# test/test_mergeoverride.py +semantic-version>2

and use this in the coresponding places in the Makefile

diff --git a/Makefile b/Makefile index 7a9be60..2855e45 100644 --- a/Makefile +++ b/Makefile @@ -101,6 +101,7 @@ testinstallvirtualenv: echo ""; \ echo " ... test install ..."; \ python -c 'import sys; from hiyapyco import __version__ as HIYAPYCOVERSION; print ("hiyapyco %s" % HIYAPYCOVERSION); print (sys.version)'; \ + pip install -r test/requirements.txt; \ make test; \ make examples; \ deactivate; \

Hello @malachib
As discussed, please remove the semantic-version dependency and if you like include test/requirements.txt stuff fromm the diff above

zerwes · 2024-08-27T03:07:28Z

hiyapyco/__init__.py

@@ -185,6 +186,12 @@ def __init__(self, *args, **kwargs):
            self.encoding = kwargs['encoding']
            del kwargs['encoding']

+        self.mergeprimitive = lambda a, b, context: b


In favor of a better readability I would prefer to use here

diff --git a/hiyapyco/__init__.py b/hiyapyco/__init__.py index d096b31..6296caf 100644 --- a/hiyapyco/__init__.py +++ b/hiyapyco/__init__.py @@ -186,7 +186,7 @@ class HiYaPyCo: self.encoding = kwargs['encoding'] del kwargs['encoding'] - self.mergeprimitive = lambda a, b, context: b + self.mergeprimitive = None if 'mergeoverride' in kwargs: # DEBT: Compare to make sure function signature is right self.mergeprimitive = kwargs['mergeoverride'] @@ -427,8 +427,12 @@ class HiYaPyCo: logger.debug('pass as b is None') pass if a is None or isinstance(b, primitiveTypes): - logger.debug('deepmerge: replace a "%s" w/ b "%s"' % (a, b,)) - a = self.mergeprimitive(a, b, context) + if self.mergeprimitive is None: + logger.debug('deepmerge: replace a "%s" w/ b "%s"' % (a, b,)) + a = b + else: + logger.debug('deepmerge: call mergeprimitive for a "%s", b "%s" with context' % (a, b,)) + a = self.mergeprimitive(a, b, context) elif isinstance(a, listTypes): if isinstance(b, listTypes): logger.debug(

Hello @malachib
you should create a new commit including the change

- self.mergeprimitive = lambda a, b, context: b + self.mergeprimitive = None

or something similar here before resolving it

zerwes · 2024-08-27T03:09:15Z

hiyapyco/__init__.py

@@ -416,7 +428,7 @@ def _deepmerge(self, a, b):
            pass
        if a is None or isinstance(b, primitiveTypes):
            logger.debug('deepmerge: replace a "%s"  w/ b "%s"' % (a, b,))
-            a = b
+            a = self.mergeprimitive(a, b, context)


see comment above.
I know it is not that fancy and pythonic, but it increases readability IMHO

if a is None or isinstance(b, primitiveTypes): - logger.debug('deepmerge: replace a "%s" w/ b "%s"' % (a, b,)) - a = self.mergeprimitive(a, b, context) + if self.mergeprimitive is None: + logger.debug('deepmerge: replace a "%s" w/ b "%s"' % (a, b,)) + a = b + else: + logger.debug('deepmerge: call mergeprimitive for a "%s", b "%s" with context' % (a, b,)) + a = self.mergeprimitive(a, b, context) elif isinstance(a, listTypes):

Hello @malachib
Please include the diff above in a new commit
Thx

malachib · 2024-08-27T14:44:05Z

@zerwes all of this strikes me as entirely reasonable. I didn't even notice a Makefile was out there, and it didn't even dawn on me that yes incurring that semver dependency on the overall library was a big whoopsie. Good catch.

I'd argue leaving self.mergeprimitive in deepmerge is cleaner, but I have to agree it's not as understandable. Maybe we can rename mergeprimitive to something like copy_or_merge?

Thanks for the props on testing :) At the risk of sounding harsh, writing code without associated unit tests is amateur hour!! no thank you!! Also I am a bit sheepish about the pseudo-semver support in the test, but it turns out merging ranges of semver is pretty hard

zerwes · 2024-08-28T05:49:57Z

@zerwes all of this strikes me as entirely reasonable. I didn't even notice a Makefile was out there, and it didn't even dawn on me that yes incurring that semver dependency on the overall library was a big whoopsie. Good catch.

I would consider your enhancement introducing mergeoverride as a plugin system. But the plugin system should not bring in all the dependencies of all (possible) plugins itself.
So in this case test/test_mergeoverride.py and your real life use case script should depend on the libs used, not hiyapyco itself.

I'd argue leaving self.mergeprimitive in deepmerge is cleaner, but I have to agree it's not as understandable. Maybe we can rename mergeprimitive to something like copy_or_merge?

You can rename mergeprimitive to what ever you consider eloquent. But for the sake of clear code and logging I would prefer to keep

         if a is None or isinstance(b, primitiveTypes):
-            logger.debug('deepmerge: replace a "%s"  w/ b "%s"' % (a, b,))
-            a = self.mergeprimitive(a, b, context)
+            if self.mergeprimitive is None:
+                logger.debug('deepmerge: replace a "%s"  w/ b "%s"' % (a, b,))
+                a = b
+            else:
+                logger.debug('deepmerge: call mergeprimitive for a "%s",  b "%s" with context' % (a, b,))
+                a = self.mergeprimitive(a, b, context)
         elif isinstance(a, listTypes):

I know, it is not that fancy and maybe not that pythonic clean, but sometimes more code is uncool, but much more readable. This way one can understand what happens here even if looking at the code 2 years later without knowledge of this PR.
Especially distinguishing the 2 cases in terms of logging is relevant in my opinion.

Thanks for the props on testing :) At the risk of sounding harsh, writing code without associated unit tests is amateur hour!! no thank you!! Also I am a bit sheepish about the pseudo-semver support in the test, but it turns out merging ranges of semver is pretty hard

Full ACK for "no test is no-go".
I must admit, I did not even have a deeper look at the test. But I can imagine it might be tricky, esp. with the comparison operators. Regarding the functionality of the test I fully trust your expertise here.

malachib · 2024-08-28T14:10:49Z

This way one can understand what happens here even if looking at the code 2 years later without knowledge of this PR.

Roger that. I respect that logic. In my own code special measures are needed sometimes to ensure that even I remember what the heck I did.

I must admit, I did not even have a deeper look at the test. But I can imagine it might be tricky, esp. with the comparison operators. Regarding the functionality of the test I fully trust your expertise here.

Thank you. I would tease that I snuck some bad code in there, but in 2024 it's too scary to even joke about that!

All in all, I am A-OK with your enhancements.

zerwes · 2024-08-28T18:40:30Z

Roger that. I respect that logic. In my own code special measures are needed sometimes to ensure that even I remember what the heck I did.

Yes, that's the point. During the 2to3 migration I had to touch many of my old scripts ... and it gave me several times some head-scratching ...

Thank you. I would tease that I snuck some bad code in there, but in 2024 it's too scary to even joke about that!

😞 Sad but true ...

All in all, I am A-OK with your enhancements.

👍 as soon as you have integrated them in your PR I can merge it and send some clacks around announcing a new release

malachib · 2024-08-28T19:42:10Z

OK, I naively clicked "resolve conversation". This is my first time adjusting a PR this way. Did I do it right? Canonical instructions left me a little flat

zerwes · 2024-08-29T07:18:57Z

OK, I naively clicked "resolve conversation". This is my first time adjusting a PR this way. ...

No problem, we all do things the first time ... and github has sometimes a own and imho sometimes strange logic...
I am far from being a github pro, in general I would include the proposed and discussed changes in a review into new commits and push them int to the PR branch; afterwards you can mark the conversation as resolved

Otherwise I would have to merge your PR into a new dev branch and commit the proposed changes by my own and then merge my own dev branch into main... but that is not reflecting the real way changes have comme in, as it is your feature and PR, not mine (i.e. not clean in terms of git history and I do not want to adorn myself with other people's laurels)

malachib · 2024-08-29T09:17:47Z

I believe I understand. Your proposed changes, being diffs, I had this idea there was a merge/accept button I could press somewhere to merge each proposed change in. Instead it appears I need to hand-recreate those in my own commits for my PR. I will do that

@zerwes

As per @zerwes recommendations

@zerwes

…ility As per @zerwes recommendations

zerwes · 2024-08-29T09:59:12Z

I had this idea there was a merge/accept button I could press somewhere to merge each proposed change in.

yes, would be nice, but there is no such feature to my knowledge (but as I noted, /me is no gh pro)

zerwes

👍 for the commit message 😄

zerwes · 2024-08-29T10:05:28Z

Thank you @malachib
Will take care of the pylint stuff later

malachib added 4 commits August 24, 2024 02:17

Initial code for primitive merge override

8aa5da3

Prepping merge override test

5cccb56

Merge override coming along

ee174a0

semver comparisons quickly becoming a complicated set operation

Cleanup + documentation

8f0880e

zerwes self-assigned this Aug 26, 2024

zerwes self-requested a review August 26, 2024 08:15

zerwes requested changes Aug 27, 2024

View reviewed changes

zerwes added the enhancement label Aug 29, 2024

malachib added 2 commits August 29, 2024 02:53

Moving semantic-version requirement to tests where it belongs

f3927e8

As per @zerwes recommendations

More explicit/less fancy mergeprimitive usage to foster code readab…

fff8cd4

…ility As per @zerwes recommendations

zerwes approved these changes Aug 29, 2024

View reviewed changes

zerwes merged commit 5af0c20 into zerwes:main Aug 29, 2024
4 of 5 checks passed

grst mentioned this pull request Oct 18, 2024

Implement none-behavior strategies #78

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override mechanism for primitive value merge #76

Override mechanism for primitive value merge #76

malachib commented Aug 25, 2024

zerwes commented Aug 27, 2024

zerwes Aug 27, 2024

zerwes Aug 29, 2024

zerwes Aug 27, 2024

zerwes Aug 29, 2024

zerwes Aug 27, 2024

zerwes Aug 29, 2024

malachib commented Aug 27, 2024 •

edited

Loading

zerwes commented Aug 28, 2024

malachib commented Aug 28, 2024 •

edited

Loading

zerwes commented Aug 28, 2024

malachib commented Aug 28, 2024

zerwes commented Aug 29, 2024

malachib commented Aug 29, 2024 •

edited

Loading

zerwes commented Aug 29, 2024

zerwes left a comment

zerwes commented Aug 29, 2024

Override mechanism for primitive value merge #76

Override mechanism for primitive value merge #76

Conversation

malachib commented Aug 25, 2024

zerwes commented Aug 27, 2024

zerwes Aug 27, 2024

Choose a reason for hiding this comment

zerwes Aug 29, 2024

Choose a reason for hiding this comment

zerwes Aug 27, 2024

Choose a reason for hiding this comment

zerwes Aug 29, 2024

Choose a reason for hiding this comment

zerwes Aug 27, 2024

Choose a reason for hiding this comment

zerwes Aug 29, 2024

Choose a reason for hiding this comment

malachib commented Aug 27, 2024 • edited Loading

zerwes commented Aug 28, 2024

malachib commented Aug 28, 2024 • edited Loading

zerwes commented Aug 28, 2024

malachib commented Aug 28, 2024

zerwes commented Aug 29, 2024

malachib commented Aug 29, 2024 • edited Loading

zerwes commented Aug 29, 2024

zerwes left a comment

Choose a reason for hiding this comment

zerwes commented Aug 29, 2024

malachib commented Aug 27, 2024 •

edited

Loading

malachib commented Aug 28, 2024 •

edited

Loading

malachib commented Aug 29, 2024 •

edited

Loading