-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement none-behavior strategies #78
Conversation
I am currently on vacation and will take care of it when I get back ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @grst
Thank you for the PR taking care of my FIXME: make None usage configurable
notes! I never found the time (and had no urgent need to) fix this.
I just have proposed some cosmetic changes, but the functionality itself LGTM. Thank you.
hiyapyco/__init__.py
Outdated
@@ -366,11 +396,21 @@ def _substmerge(self, a, b): | |||
a = copy.deepcopy(a) | |||
b = copy.deepcopy(b) | |||
logger.debug('>' * 30) | |||
logger.debug('substmerge %s and %s' % (a, b,)) | |||
# FIXME: make None usage configurable | |||
logger.debug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that this change is related to the topic of the PR and it does not improve readability ... pls revert this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, this was caused by the ruff autoformatter which I forgot to turn off. I like autoformatting a lot, especially because it prevents discussions like this during PR review, but I agree this change doesn't make sense in isolation.
hiyapyco/__init__.py
Outdated
@@ -421,11 +461,21 @@ def _deepmerge(self, a, b, context = None): | |||
if self.dereferenceyamlanchors: | |||
a = copy.deepcopy(a) | |||
b = copy.deepcopy(b) | |||
logger.debug('deepmerge %s and %s' % (a, b,)) | |||
# FIXME: make None usage configurable | |||
logger.debug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that this change is related to the topic of the PR and it does not improve readability ... pls revert this change
BTW: seems the pylint warnings/errors should be fixed by my proposed changes too ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry @grst
I assume caused by the usage of diff instead of suggest the original pass
statement has been lost ...
Co-authored-by: Klaus Zerwes <[email protected]>
Co-authored-by: Klaus Zerwes <[email protected]>
Co-authored-by: Klaus Zerwes <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, the cli diff revealed some white spaces ...
Co-authored-by: Klaus Zerwes <[email protected]>
good catch, seems I got too much used to autoformatting 😅 |
This PR adds a new option
none_behavior
that allows to specify different strategies for handlingNone
values.For now there are only two strategies implemented
NONE_BEHAVIOR_DEFAULT
-> attempt to merge or fail (previous hyapyco behavior)NONE_BEHAVIOR_OVERRIDE
->None
overrides the existing value, independent of the merge strategy.Other strategies could be implemented in the future such as ignoring None values.