Merge pull request #88 from petercorke/refactor/narrow-with-helpers

refactor: narrow optional edge ends and vertex names with a checked helper

Author: petercorke

src/pgraph/PGraph.py

@@ -1482,14 +1482,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
@@ -2273,7 +2273,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))
@@ -2339,19 +2340,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
 
@@ -2380,8 +2381,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
@@ -2627,10 +2627,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,
@@ -2656,10 +2656,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)
@@ -3443,11 +3441,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
 
@@ -3800,8 +3798,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(
@@ -3938,8 +3935,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(
@@ -3980,9 +3976,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:
         """
@@ -4035,9 +4031,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:
         """
@@ -4210,6 +4206,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
@@ -4341,12 +4358,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")
 
@@ -4385,8 +4401,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:
         """
@@ -4811,7 +4826,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):
@@ -5148,6 +5164,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:
         """

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

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