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}, " 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()