Skip to content

fix: reassigning Node.edge no longer detaches the node from its parent#235

Open
steps-re wants to merge 1 commit into
jeetsukumaran:mainfrom
steps-re:fix/node-parent-reindex
Open

fix: reassigning Node.edge no longer detaches the node from its parent#235
steps-re wants to merge 1 commit into
jeetsukumaran:mainfrom
steps-re:fix/node-parent-reindex

Conversation

@steps-re

Copy link
Copy Markdown
Contributor

Split out of #234 at @mmore500's request, so the parent/children bookkeeping change can be discussed on its own.

What it changes

Node._set_edge() (the setter behind node.edge = new_edge) removed the node from its parent's _child_nodes list as a side effect of swapping in a new Edge:

if self._parent_node is not None:
    try:
        self._parent_node._child_nodes.remove(self)
    except ValueError:
        pass

This patch removes those lines. A node's tail_node is derived from head_node._parent_node, not stored on the edge, so swapping the edge object has no bearing on where the node sits in the tree. As written, node.edge = new_edge silently dropped the node from its parent's children while node.parent_node still pointed at the old parent, leaving the tree in an inconsistent state.

The change is covered by a regression test (test_edge_setting_does_not_corrupt_parent_child_bookkeeping) asserting that after reassigning a child's edge, the parent still lists the child and the child still points back at the parent.

Why it's split from #234

#234 is a batch of small, independent static-analysis fixes. This one touches live tree-mutation semantics rather than a self-contained bug, so it warrants its own review thread. The rest of #234 (including the unrelated distance_from_root() None-parent fix in the same file) stays there.

Open question from @mmore500

Should _parent_node also be reset when the edge is reassigned? This PR only stops the node from being removed from the parent's _child_nodes list, it does not touch _parent_node. If the intended semantics of reassigning an edge are "detach this node from its current parent entirely," then both sides of the link (_parent_node and the parent's _child_nodes) should be cleared together, rather than the current half-and-half. Happy to adjust in whichever direction you prefer.

@codecov-commenter

codecov-commenter commented Jul 13, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.82%. Comparing base (e708549) to head (9c829d1).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #235      +/-   ##
==========================================
+ Coverage   88.81%   88.82%   +0.01%     
==========================================
  Files         114      114              
  Lines       12963    12975      +12     
==========================================
+ Hits        11513    11525      +12     
  Misses       1450     1450              
Flag Coverage Δ
unittests 88.82% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Node._set_edge() removed the node from its parent's _child_nodes list
as a side effect of swapping in a new Edge object, without re-adding it
anywhere or clearing _parent_node. Since a node's tail_node is derived
from head_node._parent_node (not stored independently), this detachment
had no bookkeeping purpose and only corrupted the tree: after
'node.edge = new_edge', the node vanished from its parent's children
while node.parent_node still pointed at the old parent.

Signed-off-by: Mike German <mike@stepsventures.com>
@steps-re
steps-re force-pushed the fix/node-parent-reindex branch from 7a5675e to 9c829d1 Compare July 14, 2026 15:12
@steps-re

Copy link
Copy Markdown
Contributor Author

Rebased on main to kill the conflict. Was just a collision with a sibling PR that dropped TestNodeDistanceFromRoot in the same spot. Kept both tests, squashed to one clean commit.

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.

2 participants