Add Tree.reset, which clears a Tree and then then resets the Tree.root's label and data to given values
#1709
Conversation
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
See Textualize#1437 for background. While it would be ideal to allow for the complete emptying of a Tree, the root node is required (and it's part of the construction of a Tree). So, here, when clearing the Tree we optionally allow for a new label to be given. Ideally we'll also allow for fresh data to be provided too; but there's a wrinkle there in knowing the difference between the data being None, and no data being provided (so the current root's data being carried over). Following the method of defaulting used in __init__ would cause problems. As such, rather than roll all of this into one commit, this goes with the basic requirement and the solution for data will follow. Note this also starts some unit tests for the clearing of a Tree.
Just before doing the commit I decided to rename the test tree in the new unit test for clearing down a tree. Of course I managed to name it in such a way that it becomes special to pytest. This fixes that.
|
I think the behaviour we are implementing makes sense but belongs in a new method, perhaps called If you pay me to wash your car, you don't expect to come back and see your car painted fresh with a different colour! |
src/textual/widgets/_tree.py
Outdated
| @@ -560,13 +560,20 @@ def get_label_width(self, node: TreeNode[TreeDataType]) -> int: | |||
| label = self.render_label(node, NULL_STYLE, NULL_STYLE) | |||
| return label.cell_len | |||
|
|
|||
| def clear(self) -> None: | |||
| """Clear all nodes under root.""" | |||
| def clear(self, label: TextType | None = None, data: TreeDataType = ...) -> None: | |||
There was a problem hiding this comment.
I get the limitation of not being able to clear the root and I think this is so "exceptional" that it should be made obvious from the name of the method, e.g., by renaming it to clear_children or clear_under_root.
willmcgugan
requested changes
Feb 1, 2023
There was a problem hiding this comment.
I like @rodrigogiraoserrao 's idea to have a clear method and a reset. Where clear did the bare minimum, but reset adds label and data. Feels cleaner.
It means that reset wouldn't need the ellipsis sentinel.
|
Sold. |
After some internal discussion we've decided to keep `Tree.clear` as it was, and add a `Tree.reset`, which does a `Tree.clear` but resets the label and data of `Tree.root` to the values given, while mirroring how `Tree.__init__` takes those parameters.
davep
changed the title
Tree.reset, which clears a Tree and then then resets the Tree.root's label and data to given values
willmcgugan
approved these changes
Feb 1, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
See #1437 for background. While it would be ideal to allow for the complete emptying of a Tree, the root node is required (and it's part of the construction of a Tree). So, here, when clearing the Tree we optionally allow for a new label to be given.
Ideally we'll also allow for fresh data to be provided too; but there's a wrinkle there in knowing the difference between the data being None, and no data being provided (so the current root's data being carried over). Following the method of defaulting used in init would cause problems. As such, rather than roll all of this into one commit, this goes with the basic requirement and the solution for data will follow.
Note this also starts some unit tests for the clearing of a Tree.
Edit to add: following a bit of internal chat (see comments for the conclusions), we've decided to keep
Tree.clearas ways and move this intoTree.reset, which makes things a wee bit cleaner on thedatafront too.