refactor: connect() defined once, vertex kinds supply an _attach hook

src/pgraph/PGraph.py

@@ -3176,11 +3176,14 @@ def connect(
 
         .. note::
 
-            - If the vertices subclass ``UVertex`` the edge is undirected, and if
-              they subclass ``DVertex`` the edge is directed.
+            - If the vertices subclass ``UVertex`` the edge is undirected: it is
+              recorded in the edge list of *both* vertices, so it is found from
+              either end. If they subclass ``DVertex`` the edge is directed: it
+              is recorded only in the edge list of this, the source, vertex.
             - A supplied ``edge`` built without vertices has its ``v1`` and
               ``v2`` set to the vertices being connected. One built with
-              vertices must agree with them (in either order if undirected).
+              vertices must agree with them: in either order if undirected,
+              exactly ``self`` then ``dest`` if directed.
             - Both vertices must already have been added to the same graph,
               e.g. via :meth:`PGraph.add_vertex` -- since a graph only ever
               accepts its own vertex subclass (see :meth:`UGraph.add_vertex`,
@@ -3220,8 +3223,32 @@ def connect(
         self._graph._connectivitychange = True
         self._connectivitychange = True
 
+        # the one thing that differs between undirected and directed vertices
+        self._attach(e, dest)
+
         return e
 
+    def _attach(self, edge: Edge, dest: BaseVertex) -> None:
+        """
+        Record a new edge in the vertices' edge lists (private method)
+
+        :param edge: the edge just created or supplied by :meth:`connect`, with
+            ``edge.v1`` and ``edge.v2`` set and already part of the graph
+        :param dest: the vertex ``connect`` was called with
+        :raises NotImplementedError: not provided by this vertex kind
+
+        This is the hook that :meth:`connect` calls once the connection has
+        been validated and the edge registered with the graph, so it is never
+        called for a connection that is refused.  :class:`UVertex` records the
+        edge at both ends and :class:`DVertex` at the source only.  A vertex
+        kind of your own supplies this, not :meth:`connect`, which is defined
+        once.
+        """
+        raise NotImplementedError(
+            f"{type(self).__name__} does not define how an edge is attached; "
+            "subclass UVertex or DVertex"
+        )
+
     def _endpoints_match(self, edge: Edge, dest: BaseVertex) -> bool:
         """
         Test if an edge already connects this vertex to ``dest`` (private method)
@@ -3527,68 +3554,35 @@ class UVertex(BaseVertex):
 
     This class can be inherited to provide user objects with graph capability.
 
+    :meth:`~BaseVertex.connect` adds the new edge to *both* vertices' edge
+    lists, so it is discovered from either end, and accepts a supplied edge
+    whose endpoints are the two vertices in either order.
+
+    .. runblock:: pycon
+
+        >>> from pgraph import UGraph
+        >>> g = UGraph()
+        >>> v1 = g.add_vertex(coord=[0,0], name='v1')
+        >>> v2 = g.add_vertex(coord=[1,1], name='v2')
+        >>> v1.connect(v2, cost=1.414)
+        >>> print(v1.edges())
+        >>> print(v2.edges())
 
     .. inheritance-diagram:: UVertex
 
     :seealso: :class:`BaseVertex` :class:`DVertex`
     """
 
-    def connect(
-        self,
-        dest: BaseVertex,
-        edge: Edge | None = None,
-        cost: float | None = None,
-        data: Any = None,
-    ) -> Edge:
+    def _attach(self, edge: Edge, dest: BaseVertex) -> None:
         """
-        Connect this vertex to another with an undirected edge
-
-        :param dest: vertex to connect to
-        :type dest: BaseVertex subclass
-        :param edge: Use this as the edge object, otherwise a new object of the
-                     graph's :attr:`_BaseGraph.edge_class` is created,
-                     defaults to None
-        :type edge: ``Edge`` subclass, optional
-        :param cost: the cost to traverse this edge, defaults to None
-        :type cost: float, optional
-        :param data: reference to arbitrary data associated with the edge,
-                     defaults to None
-        :type data: Any, optional
-        :raises ValueError: either vertex has not been added to a graph, the
-            vertices belong to different graphs, or ``edge`` already connects
-            other vertices
-        :raises TypeError: ``edge`` is not an instance of the graph's
-            :attr:`_BaseGraph.edge_class`
-        :return: the edge connecting the vertices
-        :rtype: Edge
-
-        Unlike the directed-graph counterpart, the new edge is added to
-        *both* vertices' edge lists, so it is discovered from either end.
-
-        If ``edge`` was built without vertices its ``v1`` and ``v2`` are set
-        to ``self`` and ``dest``. If it was built with vertices they must be
-        these two, in either order since an undirected edge has no direction.
-
-        .. runblock:: pycon
-
-            >>> from pgraph import UVertex, Edge, UGraph
-            >>> g = UGraph()
-            >>> v1 = g.add_vertex(coord=[0,0], name='v1')
-            >>> v2 = g.add_vertex(coord=[1,1], name='v2')
-            >>> v1.connect(v2, cost=1.414)
-            >>> print(v1.edges())
-            >>> print(g.edges())
-            >>> print(g)
+        Record an undirected edge at both ends (private method)
 
-        :seealso: :meth:`BaseVertex.connect` :meth:`DVertex.connect`
+        The edge is added to *both* vertices' edge lists, so it is discovered
+        from either end.  A self-loop is therefore held twice by its vertex,
+        which is why it counts 2 towards the degree.
         """
-
-        e = super().connect(dest, edge=edge, cost=cost, data=data)
-
-        self._edgelist.append(e)
-        dest._edgelist.append(e)
-
-        return e
+        self._edgelist.append(edge)
+        dest._edgelist.append(edge)
 
     def _endpoints_match(self, edge: Edge, dest: BaseVertex) -> bool:
         """
@@ -3607,65 +3601,33 @@ class DVertex(BaseVertex):
 
     This class can be inherited to provide user objects with graph capability.
 
+    :meth:`~BaseVertex.connect` adds the new edge only to *this* vertex's edge
+    list, so it is discoverable only from the start of the directed edge, and a
+    supplied edge must run exactly from this vertex to the other.
+
+    .. runblock:: pycon
+
+        >>> from pgraph import DGraph
+        >>> g = DGraph()
+        >>> v1 = g.add_vertex(coord=[0,0], name='v1')
+        >>> v2 = g.add_vertex(coord=[1,1], name='v2')
+        >>> v1.connect(v2, cost=1.414)
+        >>> print(v1.edges())
+        >>> print(v2.edges())
+
     .. inheritance-diagram:: DVertex
 
     :seealso: :class:`BaseVertex` :class:`UVertex`
     """
 
-    def connect(
-        self,
-        dest: BaseVertex,
-        edge: Edge | None = None,
-        cost: float | None = None,
-        data: Any = None,
-    ) -> Edge:
+    def _attach(self, edge: Edge, dest: BaseVertex) -> None:
         """
-        Connect this vertex to another with a directed edge
+        Record a directed edge at its source only (private method)
 
-        :param dest: vertex to connect to
-        :type dest: BaseVertex subclass
-        :param edge: Use this as the edge object, otherwise a new object of the
-                     graph's :attr:`_BaseGraph.edge_class` is created,
-                     defaults to None
-        :type edge: ``Edge`` subclass, optional
-        :param cost: the cost to traverse this edge, defaults to None
-        :type cost: float, optional
-        :param data: reference to arbitrary data associated with the edge,
-                     defaults to None
-        :type data: Any, optional
-        :raises ValueError: either vertex has not been added to a graph, the
-            vertices belong to different graphs, or ``edge`` already connects
-            other vertices
-        :raises TypeError: ``edge`` is not an instance of the graph's
-            :attr:`_BaseGraph.edge_class`
-        :return: the edge connecting the vertices
-        :rtype: Edge
-
-        Unlike the undirected-graph counterpart, the new edge is added only
-        to *this* vertex's edge list -- it is only discoverable from the
-        start of the directed edge.
-
-        If ``edge`` was built without vertices its ``v1`` and ``v2`` are set
-        to ``self`` and ``dest``. If it was built with vertices they must be
-        exactly these two, in this order, since the edge has a direction.
-
-        .. runblock:: pycon
-
-            >>> from pgraph import DVertex, Edge, DGraph
-            >>> g = DGraph()
-            >>> v1 = g.add_vertex(coord=[0,0], name='v1')
-            >>> v2 = g.add_vertex(coord=[1,1], name='v2')
-            >>> v1.connect(v2, cost=1.414)
-            >>> print(v1.edges())
-            >>> print(g.edges())
-            >>> print(g)
-
-        :seealso: :meth:`BaseVertex.connect` :meth:`UVertex.connect`
+        The edge is added only to *this* vertex's edge list, so it is
+        discoverable only from the start of the directed edge.
         """
-        e = super().connect(dest, edge=edge, cost=cost, data=data)
-
-        self._edgelist.append(e)
-        return e
+        self._edgelist.append(edge)
 
 
 # UGraph/DGraph declare their _vertex_cls here, rather than in their own

tests/test_vertex_hook.py

@@ -0,0 +1,150 @@
+"""
+``BaseVertex.connect()`` is the one public method; what differs between an
+undirected and a directed vertex is a single bookkeeping hook, ``_attach``,
+that ``connect()`` calls once the connection has been validated and the edge
+registered with the graph.
+
+This pins the design: ``connect`` is defined once, the hook is what a vertex
+kind supplies, and nothing is attached when validation fails.
+"""
+
+import unittest
+
+from pgraph import DGraph, Edge, UGraph, DVertex, UVertex
+from pgraph.PGraph import BaseVertex
+
+
+class TestConnectIsDefinedOnce(unittest.TestCase):
+    def test_subclasses_inherit_connect(self):
+        self.assertIs(UVertex.connect, BaseVertex.connect)
+        self.assertIs(DVertex.connect, BaseVertex.connect)
+
+    def test_subclasses_supply_the_hook(self):
+        self.assertIsNot(UVertex._attach, BaseVertex._attach)
+        self.assertIsNot(DVertex._attach, BaseVertex._attach)
+        self.assertIsNot(UVertex._attach, DVertex._attach)
+
+    def test_base_hook_is_not_implemented(self):
+        a, b = BaseVertex(name="a"), BaseVertex(name="b")
+        with self.assertRaises(NotImplementedError):
+            a._attach(Edge(a, b), b)
+
+
+class TestBareBaseVertex(unittest.TestCase):
+    """BaseVertex stays constructible (see policy.rst) but can never join a graph"""
+
+    def test_instantiable(self):
+        v = BaseVertex([1, 2], name="v")
+        self.assertEqual(v.name, "v")
+
+    def test_cannot_connect_or_be_added(self):
+        a, b = BaseVertex(name="a"), BaseVertex(name="b")
+        with self.assertRaises(ValueError):
+            a.connect(b)
+        with self.assertRaises(TypeError):
+            UGraph().add_vertex(a)
+        with self.assertRaises(TypeError):
+            DGraph().add_vertex(a)
+
+
+class _Hook:
+    Graph: type
+    Vertex: type
+
+    def setUp(self):
+        self.calls = []
+        outer = self
+
+        class Recording(self.Vertex):
+            def _attach(self, edge, dest):
+                # the edge is already registered with the graph at this point
+                outer.calls.append(
+                    (self, edge, dest, edge in self._graph._edgelist)
+                )
+                super()._attach(edge, dest)
+
+        self.g = self.Graph(vertex_class=Recording)
+        self.a = self.g.add_vertex([0, 0], name="a")
+        self.b = self.g.add_vertex([1, 0], name="b")
+
+    def test_attach_called_once_with_the_edge(self):
+        e = self.a.connect(self.b, cost=3)
+        self.assertEqual(len(self.calls), 1)
+        v, edge, dest, registered = self.calls[0]
+        self.assertIs(v, self.a)
+        self.assertIs(edge, e)
+        self.assertIs(dest, self.b)
+        self.assertTrue(registered)
+
+    def test_returns_the_edge(self):
+        e = self.a.connect(self.b, cost=3)
+        self.assertIsInstance(e, Edge)
+        self.assertEqual(e.cost, 3)
+        self.assertIn(e, self.g.edges())
+
+    def test_nothing_attached_when_validation_fails(self):
+        other = self.Graph().add_vertex([5, 5], name="x")
+        with self.assertRaises(ValueError):
+            self.a.connect(other)  # different graph
+        with self.assertRaises(ValueError):
+            self.a.connect(self.b, edge=Edge(self.b, self.b))  # wrong endpoints
+        ungraphed = self.Vertex([9, 9])
+        with self.assertRaises(ValueError):
+            self.a.connect(ungraphed)
+        self.assertEqual(self.calls, [])
+        self.assertEqual(self.g.ne, 0)
+        self.assertEqual((self.a.edges(), self.b.edges()), ([], []))
+
+    def test_add_edge_and_edge_connect_go_through_the_hook(self):
+        self.g.add_edge(self.a, self.b)
+        Edge(cost=1).connect(self.a, self.b)
+        self.assertEqual(len(self.calls), 2)
+
+
+class TestUndirectedHook(_Hook, unittest.TestCase):
+    Graph, Vertex = UGraph, UVertex
+
+    def test_edge_is_held_by_both_ends(self):
+        e = self.a.connect(self.b)
+        self.assertEqual(self.a.edges(), [e])
+        self.assertEqual(self.b.edges(), [e])
+
+    def test_self_loop_is_held_twice_by_its_vertex(self):
+        e = self.a.connect(self.a)
+        self.assertEqual(self.a.edges(), [e, e])
+        self.assertEqual(self.a.degree, 2)
+
+
+class TestDirectedHook(_Hook, unittest.TestCase):
+    Graph, Vertex = DGraph, DVertex
+
+    def test_edge_is_held_by_the_source_only(self):
+        e = self.a.connect(self.b)
+        self.assertEqual(self.a.edges(), [e])
+        self.assertEqual(self.b.edges(), [])
+
+    def test_self_loop_is_held_once(self):
+        e = self.a.connect(self.a)
+        self.assertEqual(self.a.edges(), [e])
+        self.assertEqual(self.a.degree, 1)
+
+
+class TestHookIsTheExtensionPoint(unittest.TestCase):
+    def test_custom_bookkeeping(self):
+        # a user kind that records the edge on the source only, though it is
+        # built on the undirected vertex: only the hook has to change
+        class OneSided(UVertex):
+            def _attach(self, edge, dest):
+                self._edgelist.append(edge)
+
+        g = UGraph(vertex_class=OneSided)
+        a = g.add_vertex([0, 0], name="a")
+        b = g.add_vertex([1, 0], name="b")
+        e = a.connect(b)
+        self.assertEqual(a.edges(), [e])
+        self.assertEqual(b.edges(), [])
+        self.assertEqual(g.ne, 1)
+
+
+if __name__ == "__main__":
+    unittest.main()