method for reloading single node of DirectoryTree by gergely-elias · Pull Request #2988 · Textualize/textual

gergely-elias · 2023-07-22T14:45:25Z

Please review the following checklist.

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

New method DirectoryTree.reload_node allows reloading the content of a single directory.

fixes #2757

rodrigogiraoserrao

This is headed in the correct direction!
Just a couple of small changes, please!

rodrigogiraoserrao · 2023-07-22T14:58:45Z

src/textual/widgets/_directory_tree.py

+        """Clear all nodes under the given node.
+
+        Returns:
+            The `Tree` instance (the subtree below the node).


You say "the subtree below the node", which doesn't sound right to me.
If you are returning self, you are returning the root tree.
If you return node, you are returning the root of the subtree that was cleared.
So, the docstring needs to be touched up.

tests/tree/test_directory_tree_reload_node.py

rodrigogiraoserrao · 2023-07-22T20:47:44Z

tests/tree/test_directory_tree_reload_node.py

+from textual.widgets import DirectoryTree
+
+
+class MyDirectoryTree(DirectoryTree):


You can use DirectoryTree directly! 😃

gergely-elias · 2023-07-23T03:53:17Z

This is headed in the correct direction! Just a couple of small changes, please!

Thanks for the comments, I'm about to push updates.
One thing I noticed is that there is a failing test case affecting tabs on windows/python3.8 setup.
I was wondering how stable the tests are and if I should look into that specific case.

davep · 2023-07-23T07:54:19Z

src/textual/widgets/_directory_tree.py

        # loading up.
        self._add_to_load_queue(self.root)

+    def clear_subtree(self, node: TreeNode[DirEntry]) -> DirectoryTree:


I think I'd have this typed to return Self.

davep · 2023-07-23T07:54:41Z

src/textual/widgets/_directory_tree.py

+
+    def reset_subtree(
+        self, node: TreeNode[DirEntry], label: TextType, data: DirEntry | None = None
+    ) -> DirectoryTree:


I think I'd have this typed to return Self.

rodrigogiraoserrao · 2023-07-23T08:18:21Z

@gergely-elias yeah some of our tests are a bit flaky; yesterday I noticed a failing test in this PR and it was completely unrelated to you. I re-ran it and it passed.
I'll let you take a look at @davep's comments and then I'll run CI again so we can merge your PR! 🚀

gergely-elias · 2023-07-23T12:08:22Z

Thanks @davep & @rodrigogiraoserrao.
I've pushed another commit, in which the import of Self is behind an if - just like the way it is done in the case of the parent class Tree.
But I saw that in other files you import Self without such condition, and it wasn't clear if there is a reason for this.

Let me know if it is fine as it is now or should be reworked.

rodrigogiraoserrao

Looks good to me! 🚀

willmcgugan

It's probably best to add some utility methods on the base case as needed here.

willmcgugan · 2023-07-26T14:11:45Z

src/textual/widgets/_directory_tree.py

+        self._tree_lines_cached = None
+        node_label = node._label
+        node_data = node.data
+        node = TreeNode(


A new TreeNode is created, but doesn't this need to be added to the tree somewhere?

you're right, the second argument should be the parent node, and not None

willmcgugan · 2023-07-26T14:13:51Z

src/textual/widgets/_directory_tree.py

        # loading up.
        self._add_to_load_queue(self.root)

+    def clear_subtree(self, node: TreeNode[DirEntry]) -> Self:


I don't think we've used the term subtree anywhere. Feels a bit out of place. Could it just be clear_node?

willmcgugan · 2023-07-26T14:18:14Z

src/textual/widgets/_directory_tree.py

+        Returns:
+            The `Tree` instance.
+        """
+        self._line_cache.clear()


We're modifying a "private" variable of the base class here, which is generally inadvisable. Too easy to inadvertently break this class if the base class is modified.

It's probably best to have any neccesary methods added to the base class.

I noticed the base class already has an invalidate_cache method, which needs just a little bit more than needed here, so in the end I did not use that one. On the other hand - as far as I can see - calling that one would also be an option here, so maybe you'd prefer to be this done with that.

Also I did not implement a utility method to increase self._updates, looked like somewhat of an overkill. But again, let me know if you'd prefer it being done like that.

willmcgugan · 2023-07-26T14:19:46Z

tests/tree/test_directory_tree_reload_node.py

+        yield DirectoryTree(self._tmp_path)
+
+
+async def test_directory_tree_reload_node(tmp_path) -> None:


Test is somewhat verbose.

Could you either A) break it in to smaller tests. or B) group some of these asserts, and add a comment that explains what is being tets

I did both break it down into two cases and added some comments. Let me know if you still think the tests should be improved.

rodrigogiraoserrao

After addressing Will's comments, the PR looks good to me.

Sorry for the time you've been waiting. We'll come back to you ASAP.

gergely-elias · 2023-08-07T13:38:35Z

@rodrigogiraoserrao no worries
I think functionality wise this PR is complete, but am not sure if I really follow all your design conventions - so if reorganizing the parts regarding cache clearing is desired, please don't hesitate to tell

willmcgugan

Looks good. Just one super trivial request.

willmcgugan · 2023-08-08T15:24:40Z

src/textual/widgets/_tree.py

        label = self.render_label(node, NULL_STYLE, NULL_STYLE)
        return label.cell_len

+    def clear_line_cache(self) -> None:


This should be private, i.e. prefixed with _. It's not something that the dev needs to know about generally.

gergely-elias · 2023-08-09T05:42:37Z

@rodrigogiraoserrao @willmcgugan
all review requests have been addressed, please approve

rodrigogiraoserrao · 2023-08-09T09:00:32Z

@gergely-elias thank you so much for all your work and patience!
I'm waiting for the tests to pass (relevant xkcd) and I'll merge it :)

willmcgugan

LGTM!

method for reloading single node of DirectoryTree

5a9e339

rodrigogiraoserrao suggested changes Jul 22, 2023

View reviewed changes

updates based on review comments

6e783d0

gergely-elias requested a review from rodrigogiraoserrao July 23, 2023 03:56

davep reviewed Jul 23, 2023

View reviewed changes

further review comments addressed

e5ef25d

rodrigogiraoserrao approved these changes Jul 23, 2023

View reviewed changes

rodrigogiraoserrao requested a review from willmcgugan July 23, 2023 12:46

willmcgugan requested changes Jul 26, 2023

View reviewed changes

gergely-elias added 5 commits July 27, 2023 21:24

Merge branch 'main' into directorytree_reload_node

6c9aeb0

replacing the term subtree with node

ed59474

preserving node's parent

5b12bcd

adding method for clearing cache to Tree class

b66d6b8

separating tests, adding explanatory comments

7e92053

gergely-elias requested a review from willmcgugan July 28, 2023 19:40

rodrigogiraoserrao approved these changes Aug 7, 2023

View reviewed changes

willmcgugan requested changes Aug 8, 2023

View reviewed changes

gergely-elias added 3 commits August 9, 2023 07:35

Merge branch 'Textualize:main' into directorytree_reload_node

cc096be

Tree.clear_line_cache renamed to Tree._clear_line_cache to mark privacy

9a3321b

CHANGELOG.md update

9a343a6

gergely-elias requested review from rodrigogiraoserrao and willmcgugan August 9, 2023 05:42

rodrigogiraoserrao approved these changes Aug 9, 2023

View reviewed changes

willmcgugan approved these changes Aug 9, 2023

View reviewed changes

rodrigogiraoserrao merged commit b7dc046 into Textualize:main Aug 9, 2023

		from textual.widgets import DirectoryTree


		class MyDirectoryTree(DirectoryTree):

		yield DirectoryTree(self._tmp_path)


		async def test_directory_tree_reload_node(tmp_path) -> None:

Uh oh!

Conversation

gergely-elias commented Jul 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gergely-elias commented Jul 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigogiraoserrao commented Jul 23, 2023

Uh oh!

gergely-elias commented Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

Uh oh!

willmcgugan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

Uh oh!

gergely-elias commented Aug 7, 2023

Uh oh!

willmcgugan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gergely-elias commented Aug 9, 2023

Uh oh!

rodrigogiraoserrao commented Aug 9, 2023

Uh oh!

willmcgugan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gergely-elias commented Jul 22, 2023 •

edited

Loading

gergely-elias commented Jul 23, 2023 •

edited

Loading