From 81855b9623163b8c52682b21f1bfb4578db5f046 Mon Sep 17 00:00:00 2001 From: Connor Stone Date: Sat, 18 Jul 2026 10:38:09 -0400 Subject: [PATCH 1/4] Allow setitem to link nodes, except for collections where behaviour is different --- src/caskade/base.py | 17 ++++++++++++----- src/caskade/collection.py | 9 ++++++++- tests/test_base.py | 10 ++++++++++ tests/test_collection.py | 31 +++++++++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 6 deletions(-) diff --git a/src/caskade/base.py b/src/caskade/base.py index 5f7a4cc..c4e1994 100644 --- a/src/caskade/base.py +++ b/src/caskade/base.py @@ -164,18 +164,22 @@ def _link(self, key: str, child: "Node"): if key in self.children: if self.children[key] is child: return - raise GraphError(f"Child key '{key}' already linked to parent {self.name}") + raise GraphError( + f"Child key '{key}' already linked to parent {self.name}, but with different node {self.children[key].name}" + ) if child in self.children.values(): - raise GraphError(f"Child {child.name} already linked to parent {self.name}") + raise GraphError( + f"Child {child.name} already linked to parent {self.name}, but not with key '{key}'" + ) if hasattr(self, key): raise LinkToAttributeError( - f"Child key '{key}' already an attribute of parent {self.name}, use a different name" + f"Child key '{key}' already an attribute of parent {self.name}, use a different name to avoid collisions" ) # avoid cycles if self in child.topological_ordering(): raise GraphError( - f"Linking {child.name} to {self.name} would create a cycle in the graph" + f"Linking {child.name} to {self.name} would create a cycle in the graph!" ) self.children[key] = child @@ -283,7 +287,7 @@ def unlink(self, key: Union[str, "Node", list, tuple, None] = None): object, the matching child is located and unlinked. If a list or tuple, each element is unlinked in turn. If ``None`` (the default), all children are unlinked. - + Raises ------ GraphError @@ -731,6 +735,9 @@ def __repr__(self) -> str: def __getitem__(self, key: str) -> "Node": return self.children[key] + def __setitem__(self, key: str, value: "Node"): + self.link(key, value) + def __eq__(self, other: "Node") -> bool: return self is other diff --git a/src/caskade/collection.py b/src/caskade/collection.py index c8bdddb..bd35865 100644 --- a/src/caskade/collection.py +++ b/src/caskade/collection.py @@ -155,6 +155,8 @@ def __init__(self, iterable=None, name=None): raise TypeError(f"NodeTuple elements must be Node objects, not {type(node)}") self.link(node) + self.link = None # Disable further linking to preserve immutability + @property def graphviz_style(self): return {"style": "solid", "color": "black", "shape": "tab"} @@ -162,8 +164,13 @@ def graphviz_style(self): def __getitem__(self, key): if isinstance(key, str): return Node.__getitem__(self, key) + if isinstance(key, slice): + return NodeTuple(tuple.__getitem__(self, key), name=self.name) return tuple.__getitem__(self, key) + def __setitem__(self, key, value): + raise TypeError("'NodeTuple' object does not support item assignment") + def __add__(self, other): res = super().__add__(other) return NodeTuple(res) @@ -264,7 +271,7 @@ def __getitem__(self, key): def __setitem__(self, key, value): self._unlink_nodes() try: - super().__setitem__(key, value) + list.__setitem__(self, key, value) finally: self._link_nodes() diff --git a/tests/test_base.py b/tests/test_base.py index 0929710..628c74e 100644 --- a/tests/test_base.py +++ b/tests/test_base.py @@ -37,6 +37,16 @@ def test_meta_link(): assert len(b.parents) == 0 +def test_linking_with_setitem(node_graph): + a, b, c, d, e, f, g = node_graph + + # Link using __setitem__ + a["new_child"] = d + assert "new_child" in a.children + assert a.children["new_child"] is d + assert a in d.parents + + def test_linking(node_graph): a, b, c, d, e, f, g = node_graph diff --git a/tests/test_collection.py b/tests/test_collection.py index f41a2b8..4e04fa6 100644 --- a/tests/test_collection.py +++ b/tests/test_collection.py @@ -284,6 +284,37 @@ def test_valid_tuple(node_tuple, params_type, group): assert backend.module.allclose(init_params[i], final_params[i]) +def test_node_tuple_immutable(): + params = [Param("p1"), Param("p2"), Param("p3")] + modules = [Module("m1"), Module("m2"), Module("m3")] + nt = NodeTuple(params + modules) + + # Attempt to modify the NodeTuple + with pytest.raises(TypeError): + nt[0] = Param("new_param") + + with pytest.raises(AttributeError): + nt.append(Param("new_param")) + + with pytest.raises(AttributeError): + nt.extend([Module("new_module")]) + + with pytest.raises(AttributeError): + nt.insert(1, Param("new_param")) + + with pytest.raises(AttributeError): + del nt[0] + + with pytest.raises(AttributeError): + nt.pop() + + with pytest.raises(AttributeError): + nt.remove(modules[0]) + + with pytest.raises(TypeError): + nt.link(Module("new_module")) + + def test_node_dict_creation(): # Minimal creation From 20544c5843eacfd9668026bd0e2cffee78212c40 Mon Sep 17 00:00:00 2001 From: "Connor Stone, PhD" Date: Sat, 18 Jul 2026 10:54:58 -0400 Subject: [PATCH 2/4] better error message for tuple with no more linking Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/caskade/collection.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/caskade/collection.py b/src/caskade/collection.py index bd35865..f631a14 100644 --- a/src/caskade/collection.py +++ b/src/caskade/collection.py @@ -155,7 +155,10 @@ def __init__(self, iterable=None, name=None): raise TypeError(f"NodeTuple elements must be Node objects, not {type(node)}") self.link(node) - self.link = None # Disable further linking to preserve immutability + def _immutable_link(*args, **kwargs): + raise TypeError("NodeTuple is immutable; cannot link new nodes after construction") + + self.link = _immutable_link # type: ignore[method-assign] @property def graphviz_style(self): From 7e9816758bddd5b2263c292f0eac2e73ccc0e70d Mon Sep 17 00:00:00 2001 From: Connor Stone Date: Sat, 18 Jul 2026 11:04:19 -0400 Subject: [PATCH 3/4] Fix review suggestions, safer linking and coverage of slicing --- src/caskade/base.py | 2 ++ src/caskade/collection.py | 3 +++ tests/test_collection.py | 5 ++++- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/caskade/base.py b/src/caskade/base.py index c4e1994..f398049 100644 --- a/src/caskade/base.py +++ b/src/caskade/base.py @@ -240,6 +240,8 @@ def link( raise NodeConfigurationError( f"key is invalid: '{key}'. Must be a valid Python identifier and not a reserved keyword." ) + if not isinstance(child, Node): + raise TypeError(f"child must be a Node object, not {type(child)}") self.__setattr__(key, child) def hierarchical_link(self, key: str, child: "Node"): diff --git a/src/caskade/collection.py b/src/caskade/collection.py index f631a14..36fc7ab 100644 --- a/src/caskade/collection.py +++ b/src/caskade/collection.py @@ -174,6 +174,9 @@ def __getitem__(self, key): def __setitem__(self, key, value): raise TypeError("'NodeTuple' object does not support item assignment") + def __delitem__(self, key): + raise TypeError("'NodeTuple' object does not support item deletion") + def __add__(self, other): res = super().__add__(other) return NodeTuple(res) diff --git a/tests/test_collection.py b/tests/test_collection.py index 4e04fa6..1a5dd69 100644 --- a/tests/test_collection.py +++ b/tests/test_collection.py @@ -52,6 +52,9 @@ def test_node_collection_creation(node_type): assert n4[3] is modules[1] assert n4[4] is modules[2] + # Make a slice + assert isinstance(n4[1:4], node_type) + # Check repr assert isinstance(repr(n4), str) assert "[5]" in repr(n4) @@ -302,7 +305,7 @@ def test_node_tuple_immutable(): with pytest.raises(AttributeError): nt.insert(1, Param("new_param")) - with pytest.raises(AttributeError): + with pytest.raises(TypeError): del nt[0] with pytest.raises(AttributeError): From dc960d00269be6aa3c8c7b80fec88aed401b7b67 Mon Sep 17 00:00:00 2001 From: Connor Stone Date: Sat, 18 Jul 2026 11:12:57 -0400 Subject: [PATCH 4/4] coverage of bad node error --- tests/test_base.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_base.py b/tests/test_base.py index 628c74e..998675b 100644 --- a/tests/test_base.py +++ b/tests/test_base.py @@ -62,6 +62,8 @@ def test_linking(node_graph): a.link("link", g) # key is attribute with pytest.raises(NodeConfigurationError): a.link("bad name", g) # Name not python identifier + with pytest.raises(TypeError): + a.link("acceptable_name", 123) # value is not a node # Double link with pytest.raises(GraphError):