From 64a82463d7f62ea1c020de97a28a4058e9e2e527 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Sun, 11 Oct 2026 15:24:33 +1000 Subject: [PATCH 1/2] test: connect() is defined once and the vertex kinds supply a single hook Fails on main: UVertex/DVertex override connect() and there is no _attach. Co-Authored-By: Claude Sonnet 5.5 --- tests/test_vertex_hook.py | 150 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 tests/test_vertex_hook.py diff --git a/tests/test_vertex_hook.py b/tests/test_vertex_hook.py new file mode 100644 index 00000000..fea4d8e9 --- /dev/null +++ b/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() From 66aff65c0bb527a4d84d039a301526aacf8f2500 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Sun, 11 Oct 2026 15:25:21 +1000 Subject: [PATCH 2/2] refactor: connect() is defined once; vertex kinds supply a single _attach hook UVertex.connect and DVertex.connect were the same sequence -- super().connect(), then append the edge to one or two edge lists -- differing only in that last line, and each repeated the full signature and docstring. A new vertex kind had to know to call super() first. BaseVertex.connect now validates, registers the edge with the graph and then calls self._attach(edge, dest). UVertex._attach records the edge at both ends, DVertex._attach at the source only, and BaseVertex._attach raises NotImplementedError. BaseVertex itself stays instantiable (policy.rst) and can still never join a graph, so the base hook is unreachable in practice. The public signature and behaviour are unchanged. The per-kind description and example move from the removed overrides to the UVertex/DVertex class docstrings and one note on BaseVertex.connect. Co-Authored-By: Claude Sonnet 5.5 --- src/pgraph/PGraph.py | 176 +++++++++++++++++-------------------------- 1 file changed, 69 insertions(+), 107 deletions(-) diff --git a/src/pgraph/PGraph.py b/src/pgraph/PGraph.py index 2afe42f2..d320faa8 100644 --- a/src/pgraph/PGraph.py +++ b/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