From eb5ff7b9f01c1dca513c0f2197de75104e78affd Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Sat, 31 Oct 2026 11:47:58 +1000 Subject: [PATCH] refactor: narrow optional edge ends and vertex names with a checked helper Fourteen of the sixteen assert statements in PGraph.py only told mypy that an optional value was set: the ends of an edge (v1 and v2 are None until it is connected) and the name of a vertex (None until it is in a graph). Bandit reports each as B101, and python -O removes them. Add Edge._ends(), which returns both vertices or raises ValueError, and Vertex._require_name(), which returns the name or raises ValueError, in the manner of Graph._require_cost(). They replace the asserts, so the narrowing is still there for mypy and the check is a real one. The two asserts left (the coordinate of a candidate in near(), the graph in Vertex.connect()) are type narrowing too, and are marked # nosec B101. A connected edge behaves as before. An unconnected one, which no graph holds, now raises ValueError from endpoints and next() instead of AssertionError (or, from next(), "should not happen"). Part of #23. Co-Authored-By: Claude Sonnet 5.5 --- src/pgraph/PGraph.py | 117 +++++++++++++++++++++++------------ tests/test_edge_endpoints.py | 37 +++++++++++ tests/test_vertex_init.py | 14 +++++ 3 files changed, 127 insertions(+), 41 deletions(-) diff --git a/src/pgraph/PGraph.py b/src/pgraph/PGraph.py index 72e1c5ac..8d7695fa 100644 --- a/src/pgraph/PGraph.py +++ b/src/pgraph/PGraph.py @@ -1459,14 +1459,14 @@ def remove_vertex(self, vertex: Vertex) -> None: """ if vertex._graph is not self: raise ValueError("vertex does not belong to this graph") - assert vertex.name is not None + name = vertex._require_name() # its own lists name every edge at it, so no need to search the graph for edge in vertex._incident_edges(): self.remove_edge(edge) self._vertexlist.remove(vertex) - del self._vertexdict[vertex.name] + del self._vertexdict[name] vertex._graph = None # it is not in any graph now vertex._label = None # and so not in a component self._connectivitychange = True @@ -2266,7 +2266,8 @@ def near(self, coord: ArrayLike, radius: float) -> list[tuple[Vertex, float]]: found = [] for vertex in candidates: - assert vertex.coord is not None # the candidates all have a position + # type narrowing only: the candidates all have a position + assert vertex.coord is not None # nosec B101 dist = float(self.metric(vertex.coord - coord)) if dist <= radius: found.append((vertex, dist)) @@ -2332,19 +2333,19 @@ def _edge_layout(self) -> tuple[dict[Edge, float], dict[Edge, int]]: groups: dict[frozenset[int], list[Edge]] = {} loops: dict[Vertex, list[Edge]] = {} for e in self._edges: - assert e.v1 is not None and e.v2 is not None - if e.v1 is e.v2: - loops.setdefault(e.v1, []).append(e) + v1, v2 = e._ends() + if v1 is v2: + loops.setdefault(v1, []).append(e) else: - groups.setdefault(frozenset((index[e.v1], index[e.v2])), []).append(e) + groups.setdefault(frozenset((index[v1], index[v2])), []).append(e) rads: dict[Edge, float] = {} for edges in groups.values(): n = len(edges) for k, e in enumerate(edges): - assert e.v1 is not None and e.v2 is not None + v1, v2 = e._ends() offset = (k - (n - 1) / 2) * _ARC_STEP # the same side in space for an edge drawn the other way round - rads[e] = offset if index[e.v1] < index[e.v2] else -offset + rads[e] = offset if index[v1] < index[v2] else -offset loopno = {e: k for edges in loops.values() for k, e in enumerate(edges)} return rads, loopno @@ -2373,8 +2374,7 @@ def _edge_artist( from matplotlib.patches import FancyArrowPatch from matplotlib.transforms import Affine2D, ScaledTranslation from ._plot import _Arrow3D, _Loop3D, _loop_path - v1, v2 = edge.v1, edge.v2 - assert v1 is not None and v2 is not None + v1, v2 = edge._ends() shrink = markersize / 2 + 1 if v1 is v2: radius = _LOOP_RADIUS + _LOOP_GROWTH * loopno @@ -2620,10 +2620,10 @@ def plot( # the edges, each once rads, loopno = self._edge_layout() for e in self._edges: - assert e.v1 is not None + start = e._ends()[0] kwargs = dict(edge_kwargs) if color_components: - kwargs = {**dict(color=color[e.v1.label, :]), **kwargs} + kwargs = {**dict(color=color[start.label, :]), **kwargs} artist = self._edge_artist( ax, e, @@ -2649,10 +2649,8 @@ def plot( ) dx, dy = abs(x1 - x0), abs(y1 - y0) for e in loopno: - assert e.v1 is not None - ax.update_datalim( - [(e.v1.x - dx, e.v1.y - dy), (e.v1.x + dx, e.v1.y + dy)] - ) + v = e._ends()[0] + ax.update_datalim([(v.x - dx, v.y - dy), (v.x + dx, v.y + dy)]) ax.autoscale_view() ax.grid(grid) @@ -3436,11 +3434,11 @@ def incidence(self) -> NDArray: vdict[vertex] = i for j, e in enumerate(edges): - assert e.v1 is not None and e.v2 is not None - I[vdict[e.v1], j] = 1 - I[vdict[e.v2], j] = 1 - if e.v1 is e.v2 and not self.directed: - I[vdict[e.v1], j] = 2 # self-loop, counted at both ends + v1, v2 = e._ends() + I[vdict[v1], j] = 1 + I[vdict[v2], j] = 1 + if v1 is v2 and not self.directed: + I[vdict[v1], j] = 2 # self-loop, counted at both ends return I @@ -3793,8 +3791,7 @@ def path_UCS( parent_names: dict[str, str] = {} for v, p in parent.items(): - assert v.name is not None and p.name is not None - parent_names[v.name] = p.name + parent_names[v._require_name()] = p._require_name() if summary or verbose: print( @@ -3931,8 +3928,7 @@ def path_Astar( parent_names: dict[str, str] = {} for v, p in parent.items(): - assert v.name is not None and p.name is not None - parent_names[v.name] = p.name + parent_names[v._require_name()] = p._require_name() if summary or verbose: print( @@ -3973,9 +3969,9 @@ def _attach(self, edge: Edge) -> None: from either end. A self-loop is therefore held twice by its vertex, which is why it counts 2 towards the degree. """ - assert edge.v1 is not None and edge.v2 is not None - edge.v1._edgelist.append(edge) - edge.v2._edgelist.append(edge) + v1, v2 = edge._ends() + v1._edgelist.append(edge) + v2._edgelist.append(edge) def _detach(self, edge: Edge) -> None: """ @@ -4028,9 +4024,9 @@ def _attach(self, edge: Edge) -> None: The edge goes in the start vertex's list of outgoing edges and in the end vertex's list of incoming ones. """ - assert edge.v1 is not None and edge.v2 is not None - edge.v1._edgelist.append(edge) - edge.v2._inlist.append(edge) + v1, v2 = edge._ends() + v1._edgelist.append(edge) + v2._inlist.append(edge) def _detach(self, edge: Edge) -> None: """ @@ -4203,6 +4199,27 @@ def _set_endpoints(self, v1: Vertex | None, v2: Vertex | None) -> None: self._v1 = v1 self._v2 = v2 + def _ends(self) -> tuple[Vertex, Vertex]: + """ + Both vertices of the edge, raising clearly if it is not connected (private method) + + :raises ValueError: the edge has no vertices + :return: the first and second vertex + :rtype: tuple of Vertex subclass + + ``v1`` and ``v2`` are None for an edge that has not been connected. + Every edge in a graph has both, so the code that works on them takes + them from here, which tells the type checker so, rather than testing + for None at each use. Unlike an ``assert`` this check is not removed + by ``python -O``. + + :seealso: :attr:`v1` :attr:`v2` + """ + v1, v2 = self._v1, self._v2 + if v1 is None or v2 is None: + raise ValueError("the edge is not connected to two vertices") + return v1, v2 + def __reduce__(self) -> Any: """ Refuse to pickle or copy an edge on its own @@ -4334,12 +4351,11 @@ def next(self, vertex: Vertex) -> Vertex: """ - if self.v1 is vertex: - assert self.v2 is not None - return self.v2 - elif self.v2 is vertex: - assert self.v1 is not None - return self.v1 + v1, v2 = self._ends() + if v1 is vertex: + return v2 + elif v2 is vertex: + return v1 else: raise ValueError("should not happen") @@ -4378,8 +4394,7 @@ def endpoints(self) -> list[Vertex]: :seealso: :meth:`vertices` """ - assert self.v1 is not None and self.v2 is not None - return [self.v1, self.v2] + return list(self._ends()) def remove(self) -> None: """ @@ -4804,7 +4819,8 @@ def _bind_edge(self, edge: Edge, dest: Vertex) -> None: endpoints disagree with the vertices' edge lists. """ graph = self._graph - assert graph is not None # connect() has checked + # type narrowing only: connect() has checked + assert graph is not None # nosec B101 if edge.v1 is None and edge.v2 is None: edge._set_endpoints(self, dest) elif not graph._endpoints_match(edge, self, dest): @@ -5141,6 +5157,25 @@ def _incident_edges(self) -> list[Edge]: seen.setdefault(id(e), e) return list(seen.values()) + def _require_name(self) -> str: + """ + The name, raising clearly if the vertex has none (private method) + + :raises ValueError: the vertex has no name + :return: the name + :rtype: str + + A vertex that is not in a graph can have no name, but one in a graph + always has, so the code that works on the vertices of a graph takes + the name from here, which tells the type checker so. Unlike an + ``assert`` this check is not removed by ``python -O``. + + :seealso: :attr:`name` + """ + if self._name is None: + raise ValueError("the vertex has no name") + return self._name + @property def name(self) -> str | None: """ diff --git a/tests/test_edge_endpoints.py b/tests/test_edge_endpoints.py index a31add97..64acfbba 100644 --- a/tests/test_edge_endpoints.py +++ b/tests/test_edge_endpoints.py @@ -120,5 +120,42 @@ def test_repr_shows_the_endpoints_and_an_edge_alone_is_not_copied(self): self.assertEqual((dup.v1.name, dup.v2.name), ("a", "b")) +class TestUnconnected(unittest.TestCase): + """The code that works on both ends of an edge says so if there are none""" + + def test_a_connected_edge_gives_its_ends(self): + g = UGraph() + a = g.add_vertex([0, 0], name="a") + b = g.add_vertex([1, 0], name="b") + e = a.connect(b) + self.assertEqual(e.endpoints, [a, b]) + self.assertIs(e.next(a), b) + self.assertIs(e.next(b), a) + + def test_an_edge_with_no_vertices_raises_clearly(self): + e = Edge() + self.assertIsNone(e.v1) + self.assertIsNone(e.v2) + v = UGraph().add_vertex([0, 0], name="a") + for name, call in ( + ("_ends", e._ends), + ("endpoints", lambda: e.endpoints), + ("next", lambda: e.next(v)), + ): + with self.subTest(name=name): + with self.assertRaises(ValueError) as cm: + call() + self.assertIn("not connected", str(cm.exception)) + + def test_next_of_a_vertex_that_is_not_an_end(self): + g = UGraph() + a = g.add_vertex([0, 0], name="a") + b = g.add_vertex([1, 0], name="b") + c = g.add_vertex([2, 0], name="c") + e = a.connect(b) + with self.assertRaises(ValueError): + e.next(c) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_vertex_init.py b/tests/test_vertex_init.py index 9a4a3cf7..2fc76b12 100644 --- a/tests/test_vertex_init.py +++ b/tests/test_vertex_init.py @@ -161,5 +161,19 @@ def test_assigning_other_attributes_is_not_a_change(self): self.assertFalse(g._spatialchange) +class TestRequireName(unittest.TestCase): + """The code that works on the vertices of a graph takes their names from here""" + + def test_a_named_vertex_gives_its_name(self): + self.assertEqual(Vertex(name="a")._require_name(), "a") + # a vertex in a graph always has one: the default is "#0", "#1", ... + self.assertEqual(UGraph().add_vertex()._require_name(), "#0") + + def test_a_vertex_with_no_name_raises_clearly(self): + with self.assertRaises(ValueError) as cm: + Vertex()._require_name() + self.assertIn("no name", str(cm.exception)) + + if __name__ == "__main__": unittest.main()