Skip to content
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

feat(tree): enable add at location #4772

Merged

Conversation

TomJGooding
Copy link
Contributor

@TomJGooding TomJGooding commented Jul 18, 2024

Add before and after parameters to the TreeNode.add and TreeNode.add_leaf methods to enable positioning a new node.

Closes #4049.

Please review the following checklist.

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

@TomJGooding TomJGooding marked this pull request as ready for review July 18, 2024 13:50
insert_index = self.children.index(before)
except ValueError:
raise AddNodeError(
"Request to add before an unknown node that is not a child of this node"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Struggled to mentally parse the error message a bit here.

Suggested change
"Request to add before an unknown node that is not a child of this node"
"The node specified for 'before' does not exist in this Tree."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually on reading the code in more detail I think my suggestion isn't clear.

Maybe something like f"cannot insert before {before}, as this node only has {len(children)} children"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I struggled a bit with how to word this error message!

The node specified for 'before' does not exist in this Tree.

Perhaps I've misunderstood, but this wording suggests the node doesn't exist in the whole Tree which might not be the case? You might just be adding to the wrong node, for example:

from textual.app import App, ComposeResult
from textual.widgets import Tree


class ExampleApp(App):
    def compose(self) -> ComposeResult:
        tree = Tree[None]("root")
        tree.root.expand()

        node_1 = tree.root.add("node 1", expand=True)
        leaf_1_1 = node_1.add_leaf("node 1.1")

        node_2 = tree.root.add("node 2", expand=True)
        leaf_2_1 = node_2.add_leaf("node 2.1")

        leaf_error = node_2.add("leaf error", after=leaf_1_1)

        yield tree


if __name__ == "__main__":
    app = ExampleApp()
    app.run()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha yeah, I realised that after posting. See my other comment 😄

Copy link
Contributor Author

@TomJGooding TomJGooding Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f"cannot insert before {before}, as this node only has {len(children)} children"

Sorry but I'm not sure that's quite right either. The length of the children isn't relevant, rather that the TreeNode passed as the before isn't a child node of this node?

"The node specified for the before node is not a child of this node" is a bit confusing to try to express clearly!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry, i thought we were in the isinstance(before, int) branch 🤦

how about:

Suggested change
"Request to add before an unknown node that is not a child of this node"
"The node specified for `before` is not a child of this node"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry also just to clarify, if the before/after argument is an index that is out of range, this also won't raise an error (the same as Python's insert)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok - that seems fine to me (I wasn't aware that's how insert worked)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that wording is much better! Updated in 04ac1ca (sorry I tried adding you as co-author but always forget that second hyphen!)

insert_index = self.children.index(after) + 1
except ValueError:
raise AddNodeError(
"Request to add after an unknown node that is not a child of this node"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Member

@darrenburns darrenburns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor request to change the wording of an error message, but otherwise looks good to me! Will be very useful - thanks!

@willmcgugan
Copy link
Collaborator

Merged the CHANGELOG.

Thanks, Tom. I think we're going to squeeze this in to 0.73.0, which I'll release at EOD.

@darrenburns darrenburns merged commit 22d0d42 into Textualize:main Jul 18, 2024
20 checks passed
@TomJGooding TomJGooding deleted the feat-tree-enable-add-at-location branch August 6, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add index to Tree widget's add
3 participants