-
-
Notifications
You must be signed in to change notification settings - Fork 653
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
Fix/gh 2680 #2711
Fix/gh 2680 #2711
Conversation
These methods intend to replace current blender() logic in a per-property level. The _resolve() method is used for non-dict properties (booleans, strings, lists), _resolve_dict() is a special-case merger for dictionaries. Also in order to support '!foo' syntax the dict_removals() function is now split in two: - dict_annihilate - dict_removals The new function dict_annihilate takes a single dict as an argument and is used by _resove_dict().
It's been replace with _resolve and _resolve_dict.
2a9285b
to
dccffb2
Compare
if self.parent is not None and hasattr(self.parent, property_name): | ||
return getattr(self.parent, property_name) | ||
elif hasattr(settings, property_name): | ||
return getattr(settings, property_name) |
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.
Well I like that but that means that we have to change some settings. For example with virt_ram
we currently have that in the settings as default_virt_ram
but we can make that work either through migrations or another if/else branch. So this is not a blocker for me. The error message below is clearly saying what the problem would be...
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.
LGTM in my eyes. New problems can be solved in other PRs but I neither see red flags or things that could fail so I am in favor of merging this.
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.
Thanks @heroin-moose!
I left some suggestions. Furthermore the line lenght should be extended to 120, especially for the comments.
traverses the tree from the object to it's topmost parent and returns | ||
the first value that is not inherited. If the the tree does not contain | ||
a value the Settings are consulted. |
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.
traverses the tree from the object to it's topmost parent and returns | |
the first value that is not inherited. If the the tree does not contain | |
a value the Settings are consulted. | |
traverses the tree from the object to its topmost parent and returns | |
the first value that is not inherited. If the tree does not contain | |
a value the settings are consulted. |
raise AttributeError("%s \"%s\" did not have the attribute \"%s\"" | ||
% (type(self), self.name, attribute_name)) | ||
AttributeError("%s \"%s\" inherits property \"%s\", but neither" | ||
" it's parent nor settings have it" |
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.
" it's parent nor settings have it" | |
" its parent nor settings have it" |
def _resolve_dict(self, property_name: str) -> dict: | ||
""" | ||
Merge the ``property_name`` dictionary of the object with the | ||
``property_name`` of all it's parents. The value of the child takes |
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.
``property_name`` of all it's parents. The value of the child takes | |
``property_name`` of all its parents. The value of the child takes |
Since this goes into my PR, I will just jump in and do @nodeg 's suggestions in my PR quickly and squash them back into your Commits @heroin-moose |
Fixes #2680