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

Add public access (both get and set) to the label of a TreeNode #1487

Merged
merged 5 commits into from
Jan 6, 2023

Conversation

davep
Copy link
Contributor

@davep davep commented Jan 5, 2023

As per #1396 this adds support for public access to the label of a TreeNode, via a label property. Both get and set access is provided. In addition to that main change, the constructor of TreeNode has been slightly modified so that the incoming label is processed via Tree.process_label to match the way that TreeNode.set_label works. Without this there was an asymmetry to how a str label was set.

davep added 2 commits January 5, 2023 09:21
Currently there's an asymmetry in how the label is handled for a TreeNode.
If a str label is passed to the constructor it stays as a str type. On the
other hand, if it's set via set_label, it gets processed into a Rich Text
type. This commit removes that asymmetry.
This adds public support to reading a TreeNode's label, and also setting it
too.

See Textualize#1396.
@davep davep added the enhancement New feature or request label Jan 5, 2023
@davep davep linked an issue Jan 5, 2023 that may be closed by this pull request
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

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

LGTM

There's going to be a whole bunch of tests relating to the Tree and TreeNode
coming, let's make sure this ends up being fairly granular.

(side thought: it might be a good time soon to revisit all the tests for
Textual and try and wrangle them into some tidy structure)
def label(self, new_label: TextType) -> TextType:
"""TextType: The label for the node."""
self.set_label(new_label)
return self.label
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, what does it mean for the setter to return the new value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, come to think of it, in Python, nothing useful at all. Hangover from elsewhere (languages where assignments are also expressions).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think C++ need that to chain assignments, like foo = bar = obj.egg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto Clipper with the likes of Class(y), for those of a particular vintage (if I'm recalling correctly).

@davep davep merged commit 83ce120 into Textualize:main Jan 6, 2023
@davep davep deleted the tree-node-label-prop branch January 6, 2023 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tree] Add label property that calls set_label
3 participants