This repository has been archived by the owner on Oct 24, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 41
Setting node name keeps tree linkage #310
Closed
etienneschalk
wants to merge
8
commits into
xarray-contrib:main
from
etienneschalk:eschalk/issue-309-tree-linkage
Closed
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
5f9c131
Setting node name keeps tree linkage
etienneschalk 5a1ac1b
what's new
etienneschalk ed0b497
Added pull to what's new
etienneschalk 3d8f4c3
Use children assignment to preserved order
etienneschalk dccd594
fast path none
etienneschalk 9a5d93b
Vertical code
etienneschalk bd3cb24
Forbid node renaming to None
etienneschalk 13df1b7
Updated failing test
etienneschalk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Instead of this orphaning then un-orphaning logic, couldn't we just change the key in
.children
directly via some logic more like in_post_attach
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 does seem immediate to me right now (I tried some things but not successfully until now)
I still need to remove then add again the node in the parent's children at some point 🤔
In all cases I get more messy code than the current solution
__delitem__
itself usesorphan
under the hood for instanceThere 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.
Finally, I went for a
.children =
assignment. I ensured the order of children is preserved by re-creating an OrderedDict from a generator on the existing parent's children. Code is more verbose, but I think this is unavoidable to ensure the order is preserved. Indeed, orphaning then re-attaching would have lost the ordering information.Note:_post_attach
solely won't rename the node_name
, so two renaming still occur:One on the parent's children (preserving order)One on the node_name
itselfEdit: if the node had a parent, there is no need to manually set the
._name
. Reassigning the updated list of children to its parents renames the node along the way