Repository navigation
refactor: narrow optional edge ends and vertex names with a checked helper - #88
Merged
Merged
Conversation
…elper 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 <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 4 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
==========================================
+ Coverage 97.98% 98.05% +0.06%
==========================================
Files 4 4
Lines 1586 1590 +4
==========================================
+ Hits 1554 1559 +5
+ Misses 32 31 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #23.
What
16
assertstatements inPGraph.py(Bandit B101) existed only to tell mypy that an optional value was set. Fourteen are now gone:assert e.v1 is not None and e.v2 is not None(11 places: edge layout, the edge artist,plot(),incidence(),_attachof both graph types,Edge.next,Edge.endpoints)v1, v2 = e._ends()assert v.name is not None(3 places:remove_vertex,path_UCS,path_Astar)v._require_name()Edge._ends()andVertex._require_name()return the value or raiseValueError, in the manner of the existingGraph._require_cost(). mypy sees the narrowed type, and the check is real: it is not removed bypython -O, which is what B101 warns about.The two asserts left (the coordinate of a candidate in
near(), the graph inVertex.connect()) are type narrowing too, with no helper that would make them clearer. They are commented as such and marked# nosec B101. I did not exclude B101 forsrc/, in line with RTB and bdsim, which exclude it only fortests/.Behaviour
Nothing changes for a connected edge or a named vertex, which is every edge and vertex in a graph. An unconnected
Edge()now raisesValueError("the edge is not connected to two vertices")from.endpointsand.next(), where it raisedAssertionError(andnext()raisedValueError("should not happen")).Performance
_attachruns for every edge added, so the helper adds a call on that path. Measured againstmain(git archive main src, three runs each, best of 7 within a run) on 90,000 edges over 30,000 vertices:UGraph.add_edgeDGraph.add_edgeNo difference beyond noise (about 1.3 µs per edge either way). My
path_UCSbenchmark (0.5 ms) found no path, so it says nothing about the search itself; the change there is only the final loop that builds the parent names.Tests
Edge._ends()/endpoints/next()on a connected edge, on an unconnectedEdge(), andnext()of a vertex that is not an endVertex._require_name()with and without a namepytest tests: 777 passed.mypy src: clean.🤖 Generated with Claude Code