fix: Edge.v1 and Edge.v2 are read-only

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

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()