refactor: graph.edges() returns a list in order of addition; both edges() return copies

src/pgraph/PGraph.py

@@ -105,7 +105,10 @@ def __init__(
         # we use a list and a dict, the list respects the order of adding
         self._vertexlist: list[BaseVertex] = []
         self._vertexdict: dict[str, BaseVertex] = {}
-        self._edgelist: set[Edge] = set()
+        # an insertion-ordered set: the keys are the edges, hashed by identity
+        # (Edge defines no __eq__), the values are unused.  This gives O(1)
+        # removal and a defined order.
+        self._edges: dict[Edge, None] = {}
         self._verbose = verbose
         self._dim = dim
         self._ncomponents = 0
@@ -377,11 +380,11 @@ def validate(self) -> list[str]:
             else:
                 problems.append(f"the vertex list holds {entry!r}, which is not a vertex")
         graph_edges: list[Edge] = []
-        for member in self._edgelist:
+        for member in self._edges:
             if isinstance(member, Edge):
                 graph_edges.append(member)
             else:
-                problems.append(f"the edge set holds {member!r}, which is not an edge")
+                problems.append(f"the graph's edges hold {member!r}, which is not an edge")
 
         # ---- vertices
         members = {id(v) for v in vertices}
@@ -844,7 +847,7 @@ def remove_edge(self, edge: Edge) -> None:
 
         :seealso: :meth:`remove_vertex` :meth:`Edge.remove`
         """
-        if edge not in self._edgelist:
+        if edge not in self._edges:
             raise ValueError("edge does not belong to this graph")
         v1, v2 = edge.v1, edge.v2
         if v1 is None or v2 is None:
@@ -860,7 +863,7 @@ def remove_edge(self, edge: Edge) -> None:
 
         edge._set_endpoints(None, None)
 
-        self._edgelist.remove(edge)
+        del self._edges[edge]
 
     def remove_vertex(self, vertex: BaseVertex) -> None:
         """
@@ -951,7 +954,7 @@ def show(self) -> None:
         for v in self._vertexlist:
             print("  " + str(v))
         print("edges:")
-        for e in self._edgelist:
+        for e in self._edges:
             print("  " + str(e))
 
     @property
@@ -989,7 +992,7 @@ def ne(self) -> int:
             >>> g.add_edge(v1, v2)
             >>> print(g.ne)
         """
-        return len(self._edgelist)
+        return len(self._edges)
 
     @abstractmethod
     def _graphcolor(self) -> int | None:
@@ -1327,21 +1330,35 @@ def closest(self, coord: ArrayLike) -> tuple[BaseVertex | None, float]:
 
         return min_which, min_dist
 
-    def edges(self) -> set[Edge]:
+    def edges(self) -> list[Edge]:
         """
         Get all edges in graph (base class method)
 
         :return: All edges in the graph
-        :rtype: set of Edge references
+        :rtype: list of Edge references
+
+        The edges are in the order they were added to the graph.  This is a
+        new list, so it can be modified or sorted without affecting the graph,
+        and edges can be removed from the graph while iterating over it.
 
         We can iterate over all edges in the graph by::
 
             for e in g.edges():
                 print(e)
 
-        .. note:: Unlike :meth:`BaseVertex.edges`, which returns a ``list`` in
-            connection order, this returns a ``set`` with no defined
-            iteration order.
+        and select one by index, for example the second edge added is
+        ``g.edges()[1]``.
+
+        .. note:: Like :meth:`BaseVertex.edges` this is a ``list``, but where
+            that lists the edges attached to one vertex, this lists every edge
+            in the graph once.  A self-loop is one edge here, but appears
+            twice in the :meth:`BaseVertex.edges` of an undirected vertex
+            because it has two ends there.
+
+        .. versionchanged:: 1.0
+            Returns a ``list`` in order of addition.  In 0.6.5 this was the
+            graph's own ``set``, with no defined order, although the
+            documentation said ``list``.
 
         .. runblock:: pycon
 
@@ -1352,9 +1369,9 @@ def edges(self) -> set[Edge]:
             >>> g.add_edge(v1, v2)
             >>> print(g.edges())
 
-        :seealso: :meth:`BaseVertex.edges`
+        :seealso: :meth:`BaseVertex.edges` :attr:`ne`
         """
-        return self._edgelist
+        return list(self._edges)
 
     def plot(
         self,
@@ -2224,14 +2241,14 @@ def incidence(self) -> NDArray:
         .. note::
             - vertices are numbered in their order of creation. A vertex index
               can be resolved to a vertex reference by ``graph[i]``.
-            - edges are numbered in the order they appear in ``graph.edges()``.
+            - edges are numbered in the order they appear in :meth:`edges`, which
+              is the order they were added.
             - Both endpoints of every edge are marked, regardless of
               direction -- for a ``DGraph`` this means a vertex that is only
-              ever a target (never a source) still appears here, unlike
-              :meth:`degree`/:attr:`BaseVertex.degree`, which count outgoing
-              edges only. Iterating each vertex's own
-              :meth:`BaseVertex.edges` instead would silently drop such
-              vertices for a directed graph.
+              ever a target (never a source) still appears here.  Iterating
+              each vertex's own :meth:`BaseVertex.edges`, which are its
+              outgoing edges, would silently drop such vertices for a
+              directed graph.
 
         .. rubric:: For graphs with parallel edges or loops
 
@@ -3541,7 +3558,7 @@ def connect(
         else:
             e = self._graph._edge_class(self, dest, cost=cost, data=data)
 
-        self._graph._edgelist.add(e)
+        self._graph._edges[e] = None
         self._graph._connectivitychange = True
         self._connectivitychange = True
 
@@ -3661,11 +3678,16 @@ def edges(self) -> list[Edge]:
         :return: List of all edges leaving this vertex
         :rtype: list of Edge
 
+        The edges are in the order they were connected.  This is a new list,
+        so it can be modified, and edges can be removed from the graph while
+        iterating over it.
+
         .. note::
 
             - For a directed graph the edges are those leaving this vertex
             - For a non-directed graph the edges are those leaving or entering
-                this vertex
+                this vertex, and a self-loop is listed twice, once for each of
+                its ends
 
         .. runblock:: pycon
 
@@ -3679,7 +3701,7 @@ def edges(self) -> list[Edge]:
         :seealso: :meth:`out_edges` :meth:`in_edges` :meth:`adjacent`
             :meth:`incidences` :attr:`degree` :meth:`BaseGraph.edges`
         """
-        return self._edgelist
+        return list(self._edgelist)
 
     def heuristic_distance(self, v2: BaseVertex) -> float:
         """

src/pgraph/exporters.py

@@ -141,12 +141,11 @@ def _ordered_edges(graph: BaseGraph) -> list[tuple[Edge, BaseVertex, BaseVertex]
     """
     Edges in a reproducible order
 
-    ``graph.edges()`` is a set, so its order changes from run to run and the
-    exported text would not be stable (useless for diffs and golden files).
-    Instead walk the vertices in creation order and each vertex's own
-    outgoing edges, keeping the first sight of every edge.  Identity is by object, not by
-    endpoints, so parallel edges are all kept, and an undirected edge (held by
-    both ends, twice for a self-loop) is listed once.
+    Walk the vertices in creation order and each vertex's own outgoing edges,
+    keeping the first sight of every edge, so that edges are grouped by the
+    vertex they leave.  Identity is by object, not by endpoints, so parallel
+    edges are all kept, and an undirected edge (held by both ends, twice for a
+    self-loop) is listed once.
 
     Each item is ``(edge, start, end)``.
     """

tests/test_edges_list.py

@@ -0,0 +1,97 @@
+"""``graph.edges()`` and ``vertex.edges()`` return new lists in order of addition.
+
+In 0.6.5 ``graph.edges()`` returned the graph's own ``set`` (documented as a
+list), so it had no order, and either method returned the live container, so
+a caller could corrupt the graph by modifying the result.
+"""
+
+import unittest
+
+from pgraph import DGraph, UGraph
+
+
+class _EdgesList:
+    cls: type
+
+    def setUp(self):
+        self.g = self.cls()
+        self.a, self.b, self.c = (self.g.add_vertex() for _ in range(3))
+        self.e1 = self.g.add_edge(self.a, self.b)
+        self.e2 = self.g.add_edge(self.b, self.c)
+        self.e3 = self.g.add_edge(self.a, self.c)
+
+    def test_is_a_list_in_order_of_addition(self):
+        edges = self.g.edges()
+        self.assertIsInstance(edges, list)
+        self.assertEqual(edges, [self.e1, self.e2, self.e3])
+
+    def test_order_survives_removal(self):
+        self.g.remove_edge(self.e2)
+        e4 = self.g.add_edge(self.b, self.c)
+        self.assertEqual(self.g.edges(), [self.e1, self.e3, e4])
+
+    def test_graph_result_is_a_copy(self):
+        self.g.edges().clear()
+        self.assertEqual(self.g.ne, 3)
+        self.assertEqual(self.g.edges(), [self.e1, self.e2, self.e3])
+        self.assertEqual(self.g.validate(), [])
+
+    def test_vertex_result_is_a_copy(self):
+        self.a.edges().clear()
+        self.assertEqual(len(self.a.edges()), 2)
+        self.assertEqual(self.g.validate(), [])
+
+    def test_remove_while_iterating_visits_every_edge(self):
+        seen = []
+        for e in self.g.edges():
+            seen.append(e)
+            self.g.remove_edge(e)
+        self.assertEqual(seen, [self.e1, self.e2, self.e3])
+        self.assertEqual(self.g.ne, 0)
+
+    def test_vertex_remove_while_iterating_visits_every_edge(self):
+        seen = []
+        for e in self.a.edges():
+            seen.append(e)
+            self.g.remove_edge(e)
+        self.assertEqual(seen, [self.e1, self.e3])
+        self.assertEqual(self.a.edges(), [])
+
+    def test_parallel_edges_are_both_listed(self):
+        e4 = self.g.add_edge(self.a, self.b)
+        self.assertEqual(self.g.edges().count(e4), 1)
+        self.assertEqual(len(self.g.edges()), 4)
+        self.assertEqual(self.g.ne, 4)
+
+    def test_loop_is_one_graph_edge(self):
+        loop = self.g.add_edge(self.c, self.c)
+        self.assertEqual(self.g.edges().count(loop), 1)
+        self.assertEqual(self.g.ne, 4)
+
+    def test_edge_removed_via_edge_leaves_the_list(self):
+        self.e1.remove()
+        self.assertEqual(self.g.edges(), [self.e2, self.e3])
+
+    def test_vertex_removal_removes_its_edges(self):
+        self.g.remove_vertex(self.b)
+        self.assertEqual(self.g.edges(), [self.e3])
+
+
+class TestUGraphEdgesList(_EdgesList, unittest.TestCase):
+    cls = UGraph
+
+    def test_loop_is_listed_twice_at_its_vertex(self):
+        loop = self.g.add_edge(self.c, self.c)
+        self.assertEqual(self.c.edges().count(loop), 2)
+
+
+class TestDGraphEdgesList(_EdgesList, unittest.TestCase):
+    cls = DGraph
+
+    def test_loop_is_listed_once_at_its_vertex(self):
+        loop = self.g.add_edge(self.c, self.c)
+        self.assertEqual(self.c.edges().count(loop), 1)
+
+
+if __name__ == "__main__":
+    unittest.main()

tests/test_multigraph.py

@@ -1,7 +1,7 @@
 """
 Self-loops and parallel edges.
 
-pgraph does not forbid either, so every quantity derived from the edge set
+pgraph does not forbid either, so every quantity derived from the graph's edges
 (degree, the matrix representations, cycle detection, path planning) has to
 give a defined, consistent answer when they are present.
 

tests/test_validate.py

@@ -178,7 +178,7 @@ def test_junk_in_the_containers_is_reported_not_raised(self):
         # validate() is for finding corruption, so it must survive it
         g, a, b, c, d = build(UGraph)
         g._vertexlist.append("junk")
-        g._edgelist.add(42)
+        g._edges[42] = None
         a._edgelist.append("junk")
         msg = self.problems(g)
         for word in ("junk", "42", "not a vertex", "not an edge"):
@@ -209,14 +209,14 @@ class MyEdge(Edge):
         g = UGraph(edge_class=MyEdge)
         a, b = g.add_vertex(name="a"), g.add_vertex(name="b")
         e = Edge(a, b)
-        g._edgelist.add(e)
+        g._edges[e] = None
         a._edgelist.append(e)
         b._edgelist.append(e)
         self.assertIn("MyEdge", self.problems(g))
 
     def test_vertex_list_entry_for_an_edge_removed_from_the_graph(self):
         g, a, b, c, d = build(UGraph)
-        g._edgelist.discard(a.edges()[0])
+        g._edges.pop(a.edges()[0], None)
         self.assertIn("not in the graph", self.problems(g))
 
     def test_self_loop_held_once_instead_of_twice(self):

tests/test_vertex_hook.py

@@ -59,7 +59,7 @@ 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)
+                    (self, edge, dest, edge in self._graph._edges)
                 )
                 super()._attach(edge, dest)