From 190b21ed3a558ae5a258b7dc2fcc14860fc329a1 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 08:27:06 +1000 Subject: [PATCH 1/2] test: Edge.v1 and Edge.v2 are read-only; the library still binds them Co-Authored-By: Claude Sonnet 5.5 --- tests/test_edge_endpoints.py | 121 +++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 tests/test_edge_endpoints.py diff --git a/tests/test_edge_endpoints.py b/tests/test_edge_endpoints.py new file mode 100644 index 00000000..ed6a84f5 --- /dev/null +++ b/tests/test_edge_endpoints.py @@ -0,0 +1,121 @@ +""" +``Edge.v1`` and ``Edge.v2`` are read-only. + +They used to be plain attributes, so a caller could write ``e.v1 = None`` on an +edge that was in a graph and leave it wedged: ``remove_edge`` and +``remove_vertex`` then raised and the edge could not be removed. Only the +library binds an edge to its vertices -- the constructor, ``connect()`` and +``remove_edge()`` -- so an edge in a graph always has both of its ends. +""" + +import copy +import unittest + +from pgraph import DGraph, Edge, UGraph + + +class TestReadOnly(unittest.TestCase): + def setUp(self): + self.g = UGraph() + self.a = self.g.add_vertex([0, 0], name="a") + self.b = self.g.add_vertex([1, 0], name="b") + self.c = self.g.add_vertex([2, 0], name="c") + self.e = self.a.connect(self.b) + + def test_cannot_assign(self): + for name, value in (("v1", None), ("v1", self.c), ("v2", None), ("v2", self.c)): + with self.subTest(name=name, value=value): + with self.assertRaises(AttributeError) as cm: + setattr(self.e, name, value) + # the message says what to do instead + self.assertIn("connect", str(cm.exception)) + + def test_cannot_delete(self): + with self.assertRaises(AttributeError): + del self.e.v1 + + def test_failed_assignment_changes_nothing(self): + with self.assertRaises(AttributeError): + self.e.v1 = self.c + self.assertIs(self.e.v1, self.a) + self.assertIs(self.e.v2, self.b) + self.assertEqual(self.a.edges(), [self.e]) + self.assertEqual(self.c.edges(), []) + + def test_the_graph_can_always_remove_its_edges(self): + for name in ("v1", "v2"): + try: + setattr(self.e, name, None) + except AttributeError: + pass + self.g.remove_edge(self.e) # not wedged + self.assertEqual(self.g.ne, 0) + self.g.remove_vertex(self.a) + self.assertEqual(self.g.n, 2) + + +class TestLibraryStillBindsThem(unittest.TestCase): + def test_constructor(self): + g = DGraph() + a, b = g.add_vertex(name="a"), g.add_vertex(name="b") + e = Edge(a, b, cost=3) + self.assertIs(e.v1, a) + self.assertIs(e.v2, b) + + def test_standalone_edge_has_no_vertices(self): + e = Edge(cost=1) + self.assertIsNone(e.v1) + self.assertIsNone(e.v2) + + def test_connect_binds_a_bare_edge(self): + g = UGraph() + a, b = g.add_vertex(name="a"), g.add_vertex(name="b") + e = Edge(cost=1) + a.connect(b, edge=e) + self.assertEqual((e.v1, e.v2), (a, b)) + + def test_edge_connect_binds(self): + g = UGraph() + a, b = g.add_vertex(name="a"), g.add_vertex(name="b") + e = Edge(cost=1) + e.connect(a, b) + self.assertEqual((e.v1, e.v2), (a, b)) + + def test_removal_unlinks_and_the_edge_can_be_reused(self): + g = UGraph() + a, b, c = (g.add_vertex(name=n) for n in "abc") + e = a.connect(b) + g.remove_edge(e) + self.assertIsNone(e.v1) + self.assertIsNone(e.v2) + with self.assertRaises(ValueError): + g.add_edge(e) # unlinked, so add_edge(edge) refuses it + b.connect(c, edge=e) # but connect() re-binds it + self.assertEqual((e.v1, e.v2), (b, c)) + self.assertEqual(g.ne, 1) + + def test_subclass_that_calls_super(self): + class Observation(Edge): + def __init__(self, v1, v2, uv): + super().__init__(v1, v2, cost=0) + self.uv = uv + + g = UGraph() + a, b = g.add_vertex(name="a"), g.add_vertex(name="b") + o = Observation(a, b, (1, 2)) + a.connect(b, edge=o) + self.assertEqual((o.v1, o.v2, o.uv), (a, b, (1, 2))) + + def test_repr_and_copies_keep_the_endpoints(self): + g = UGraph() + a, b = g.add_vertex(name="a"), g.add_vertex(name="b") + e = a.connect(b, cost=2) + self.assertIn("a", str(e)) + for dup in (copy.copy(e), copy.deepcopy(e)): + self.assertEqual(dup.cost, 2) + self.assertIsNotNone(dup.v1) + self.assertIsNotNone(dup.v2) + + +if __name__ == "__main__": + unittest.main() From d8f7911b722cf2121ca0a4f24938481b81196c13 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 08:27:08 +1000 Subject: [PATCH 2/2] fix: Edge.v1 and Edge.v2 are read-only They were plain attributes, so e.v1 = None on an edge that was in a graph left it wedged: remove_edge and remove_vertex raised, incidence() hit an assert, and the edge could not be removed. Assigning to one of them to another vertex left the edge disagreeing with the vertices' edge lists. Only the library binds an edge to its vertices -- the constructor, connect(), and remove_edge() -- so they are now properties over private fields, set through one method, and assigning raises AttributeError saying to use the constructor or connect() instead. An edge that is in a graph always has both of its ends. The downstream Edge subclasses (MVTB Observation, RTB LatticeEdge and PGEdge) all call super().__init__(v1, v2), and nothing in RTB, MVTB, RVC3 or spatialmath assigns an endpoint. Co-Authored-By: Claude Sonnet 5.5 --- src/pgraph/PGraph.py | 75 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 65 insertions(+), 10 deletions(-) diff --git a/src/pgraph/PGraph.py b/src/pgraph/PGraph.py index 91852129..308b7965 100644 --- a/src/pgraph/PGraph.py +++ b/src/pgraph/PGraph.py @@ -642,8 +642,7 @@ def remove_edge(self, edge: Edge) -> None: v2._connectivitychange = True self._connectivitychange = True - edge.v1 = None - edge.v2 = None + edge._set_endpoints(None, None) self._edgelist.remove(edge) @@ -2629,6 +2628,12 @@ def _graphcolor(self) -> int | None: # ========================================================================== # +_ENDPOINT_MESSAGE = ( + "Edge.{0} is read-only. Give the vertices to the constructor, Edge(v1, v2), " + "or connect the edge with v1.connect(v2, edge=e) or e.connect(v1, v2)" +) + + class Edge: """ Edge class @@ -2638,8 +2643,8 @@ class Edge: Each edge has: - ``cost`` cost of traversing this edge, required for planning methods - ``data`` reference to arbitrary data associated with the edge - - ``v1`` first vertex, start vertex for a directed edge - - ``v2`` second vertex, end vertex for a directed edge + - ``v1`` first vertex, start vertex for a directed edge (read-only) + - ``v2`` second vertex, end vertex for a directed edge (read-only) .. note:: @@ -2696,8 +2701,8 @@ def __init__( :seealso: :meth:`Edge.connect` :meth:`BaseVertex.connect` """ - self.v1 = v1 - self.v2 = v2 + self._v1 = v1 + self._v2 = v2 self.data = data @@ -2717,6 +2722,58 @@ def __init__( else: self.cost = None + @property + def v1(self) -> BaseVertex | None: + """ + First vertex of the edge, the start vertex for a directed edge + + :return: the vertex, or None if the edge is not connected + :rtype: BaseVertex subclass or None + + .. versionchanged:: 1.0 + Read-only. It was a plain attribute, so assigning to it on an edge + that was in a graph left the graph inconsistent: the vertices' + edge lists no longer agreed with the edge, and the edge could not + be removed. Give the vertices to the constructor, ``Edge(v1, + v2)``, or to :meth:`connect` or :meth:`BaseVertex.connect`. + + :seealso: :attr:`v2` :meth:`connect` + """ + return self._v1 + + @v1.setter + def v1(self, value: BaseVertex | None) -> None: + raise AttributeError(_ENDPOINT_MESSAGE.format("v1")) + + @property + def v2(self) -> BaseVertex | None: + """ + Second vertex of the edge, the end vertex for a directed edge + + :return: the vertex, or None if the edge is not connected + :rtype: BaseVertex subclass or None + + Read-only, see :attr:`v1`. + + :seealso: :attr:`v1` :meth:`connect` + """ + return self._v2 + + @v2.setter + def v2(self, value: BaseVertex | None) -> None: + raise AttributeError(_ENDPOINT_MESSAGE.format("v2")) + + def _set_endpoints(self, v1: BaseVertex | None, v2: BaseVertex | None) -> None: + """ + Bind the edge to its vertices, or unbind it (private method) + + Only the library calls this: the constructor's own assignment, then + :meth:`connect`, :meth:`BaseVertex.connect` for an edge built without + vertices, and :meth:`_BaseGraph.remove_edge`, which clears them. + """ + self._v1 = v1 + self._v2 = v2 + def __repr__(self) -> str: """ Detailed representation of the edge @@ -2804,8 +2861,7 @@ def connect(self, v1: BaseVertex, v2: BaseVertex) -> None: raise ValueError("vertices must belong to the same graph") # connect edge to its vertices - self.v1 = v1 - self.v2 = v2 + self._set_endpoints(v1, v2) # tell the vertices to add edge to their edgelists as appropriate for # DGraph or UGraph @@ -3279,8 +3335,7 @@ def _bind_edge(self, edge: Edge, dest: BaseVertex) -> None: endpoints disagree with the vertices' edge lists. """ if edge.v1 is None and edge.v2 is None: - edge.v1 = self - edge.v2 = dest + edge._set_endpoints(self, dest) elif not self._endpoints_match(edge, dest): raise ValueError( f"edge already connects {edge.v1} and {edge.v2}, "