-
Notifications
You must be signed in to change notification settings - Fork 56
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
add removal of nodes / branches v2 #348
Conversation
Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
hi @erikbosch @ppb2020 created this squashed and rebased PR, it includes all changes that we spoke about in PR #342 |
Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
MoM:
|
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.
A few additional comments. The PR can go in as is with my comments being addressed in the future, if this is simpler for you.
delete: true | ||
``` | ||
|
||
Also, the delement element can be used on instances after they have been expanded. If you want to delete a node or |
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 neat that this works, BTW. Cool!
As a side question, what kind of error is generated if one tries to delete a non-existing instance? Is it differentiated from an error message generated by trying to delete a node that has no instances?
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.
done even added a test for this case, it fails at this position in the code:
...
# Type presence should have been tested earlier, but is tested here again for completeness
if "type" not in self.source_dict.keys():
logging.error("Invalid VSS element %s, must have type", self.name)
sys.exit(-1)
Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
Should be ready for merge now |
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.
Looks good to me, waiting until Friday to see if anyone else has any comments
* add 'delete' element for node/branch removal Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
This is a squashed and rebased version of PR #342, please follow the link for more details