Skip to content

Route NestedTree's collapse calls through one loop - #110

Merged
pazz merged 1 commit into
mainfrom
nested-collapse-shrink
Oct 25, 2026
Merged

pazz merged 1 commit into
mainfrom
nested-collapse-shrink

Conversation

@pazz

@pazz pazz commented Oct 25, 2026

Copy link
Copy Markdown
Owner

NestedTree.collapsible, is_collapsed, toggle_collapsed, collapse and expand each looked up the tree managing the position and checked that it can collapse. The new private _on_subtree does that once and replaces _get_subtree_for; each method is one line. Found by an audit for over-engineering.

When NestedTrees are wrapped directly in each other, these methods used to call themselves once per level and failed at about a thousand levels. _on_subtree walks down to the managing tree in a loop, so there is no such limit now. test_collapse_deeply_nested collapses through 1100 levels; it fails on main.

Return values are unchanged: collapsible and is_collapsed default to False, the other three return None. A seeded differential test over 30 random nested trees with all collapse calls gives identical results on main and this branch.

Review findings and resolution

review-correctness

  • The first version recursed twice per directly nested NestedTree (method and helper), halving the depth that worked: fixed, by walking in a loop, which removes the recursion altogether; regression test added.
  • The private _get_subtree_for is removed: left; private and unused elsewhere.
  • Checked without findings: return values, the differential test above, 10000 sibling subtrees, the full suite on urwid 3.0.4 and 2.1.2.

review-design

  • String dispatch through getattr is slightly harder to grep than direct calls: left; a helper returning the tree would be the same size, and the reviewer did not require it.
  • Whether this deserves its own PR: kept separate, as it is a different module from the other cleanups.

🤖 Generated with Claude Code

collapsible, is_collapsed, toggle_collapsed, collapse and expand each
looked up the tree managing the position and checked that it can
collapse; _on_subtree now does that once, replacing _get_subtree_for.
It walks down through NestedTrees nested directly in each other in a
loop, where the methods used to recurse once per level.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@pazz
pazz merged commit 76080b7 into main Oct 25, 2026
4 checks passed
@pazz
pazz deleted the nested-collapse-shrink branch October 25, 2026 11:31
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.

1 participant