Add read-only access to the children of a TreeNode
#1495
Conversation
In anticipation of satisfying Textualize#1398, this adds a generic immutable sequence wrapper class. The idea being that it can be used to wrap up a list or similar, that you don't want the caller to modify. This commit aims to get the basics down for this, and also adds a minimal set of unit tests.
Make it 100% clear that the tests that are about wrapping iterators actually are wrapping iterators.
There was a problem hiding this comment.
re naming, I think this is essentially a view. Much like the dict views.
Suggest ImmutableSequenceView as a name. I don't know if it qualifies as a collection, so I think maybe just name the file "_immutable_sequence_view.py" for the moment.
Since the use case is to provide a immutable view on to a collection. It shouldn't copy the data internally. Let's store a reference to the Sequence parameter supplied in the constructor.
Makes sense, I'll make those changes.
I'm not sure I quite follow this though. As the code stands doesn't it store a reference if it's an indexable sequence type, and only retains a copy if the incoming sequence is one that can't be indexed, allowing it ti be indexed. Perhaps I've misunderstood what was asked for here? |
It's only ever going to be used as a shim on top of a sequence. Can't see any use for creating these for iterators. |
Ahh, okay, that makes life easier. I only took that into account because you asked that it do so (accept an iterator too) when we spoke about this yesterday. Happy to drop that part. |
Also, in doing so, drop support for unrolling iterators and making them into indexable sequences. See the following feedback: Textualize#1495 (review) Textualize#1495 (comment)
|
Right, that should be those couple or so issues addressed. Thanks. |
|
|
||
| def test_tree_node_children() -> None: | ||
| """A node's children property should act like an immutable list.""" | ||
| CHILDREN=23 |
There was a problem hiding this comment.
Did you run Black on this?
There was a problem hiding this comment.
Not by hand on the tests, no. black is run by pre-commit of course so if there's a non-black-happy-ness in the above, that would suggest that both pre-commit and CI black checks avoid the unit tests?
Checking:
$ poetry run black --check .
would reformat devtools/__init__.py
would reformat snapshot_tests/snapshot_apps/horizontal_auto_width.py
would reformat snapshot_tests/snapshot_apps/multiple_css/multiple_css.py
would reformat renderables/test_sparkline.py
would reformat snapshot_tests/snapshot_apps/key_display.py
would reformat renderables/test_text_opacity.py
would reformat test_path.py
would reformat test_focus.py
would reformat test_immutable_sequence_view.py
would reformat test_node_list.py
would reformat renderables/test_underline_bar.py
would reformat test_arrange.py
would reformat test_unmount.py
would reformat test_table.py
would reformat test_dom.py
would reformat test_screens.py
would reformat tree/test_tree_node_label.py
would reformat tree/test_tree_node_children.py
would reformat test_text_backend.py
would reformat test_widget_child_moving.py
would reformat test_reactive.py
would reformat test_widget_mounting.py
would reformat test_widget_removing.py
would reformat test_geometry.py
Oh no! 💥 💔 💥
24 files would be reformatted, 66 files would be left unchanged.Guess none of us are?
There was a problem hiding this comment.
I think we may be excluding tests.
Can you check you are also running the latest black specified in dev dependencies.
There was a problem hiding this comment.
$ poetry run black --version
black, 22.10.0 (compiled: yes)
Python (CPython) 3.11.1Which seems to be a later version that specified in poetry.lock?
[[package]]
name = "black"
version = "22.8.0"
Although weirdly:
black = "^22.3.0,<22.10.0" # macos wheel issue on 22.10
from pyproject.toml
Also, in doing so, drop support for unrolling iterators and making them into indexable sequences. See the following feedback: #1495 (review) #1495 (comment)
Adds a generic
ImmutableSequencewrapper class (name and location totally up for debate, although the name of the class got the nod in a Slack chat so I suspect that's okay) and then goes on to use that to wrap the internal child list of aTreeNode, making it available in a non-destructive way.See #1398.