-
Notifications
You must be signed in to change notification settings - Fork 57
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
Pr433 #435
Pr433 #435
Conversation
Signed-off-by: Daniel Alvarez-Coello <[email protected]>
Signed-off-by: Erik Jaegervall <[email protected]>
@jdacoello - Can you check the changes in the commit I added on top, if they looks ok. If so I can merge this PR and close yours, or if you prefer you can incorporate the changes from this PR into yours. |
They seem to be ok. You can go ahead and merge this PR and close the other one. I forgot that I changed a little the doc strings and the order in which elements appear in the resulting graphql schema. Apparently, that is why the tests were not passing. Thanks for addressing that and updating the expected files. |
pass | ||
|
||
|
||
def get_instance_root(root: VSSNode, depth: int = 1) -> tuple[VSSNode, int]: |
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.
The go exporter initially defined that one. Should be removed from the go exporter then
raise NoInstanceRootException() | ||
|
||
|
||
def add_children_map_entries(root: VSSNode, fqn: str, replace: str, map: dict[str, str]) -> None: |
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.
Same here
add_children_map_entries(child, fqn, replace, map) | ||
|
||
|
||
def get_instance_mapping(root: VSSNode | None) -> dict[str, str]: |
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.
And here
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.
This PR is merged now - are you interested in doing the cleanup in a new PR?
Same as #433 but trying to fix build errors