diff --git a/src/caskade/base.py b/src/caskade/base.py index 5f7a4cc..f398049 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 @@ -236,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"): @@ -283,7 +289,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 +737,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..36fc7ab 100644 --- a/src/caskade/collection.py +++ b/src/caskade/collection.py @@ -155,6 +155,11 @@ def __init__(self, iterable=None, name=None): raise TypeError(f"NodeTuple elements must be Node objects, not {type(node)}") self.link(node) + 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): return {"style": "solid", "color": "black", "shape": "tab"} @@ -162,8 +167,16 @@ 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 __delitem__(self, key): + raise TypeError("'NodeTuple' object does not support item deletion") + def __add__(self, other): res = super().__add__(other) return NodeTuple(res) @@ -264,7 +277,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..998675b 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 @@ -52,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): diff --git a/tests/test_collection.py b/tests/test_collection.py index f41a2b8..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) @@ -284,6 +287,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(TypeError): + 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