merge networkx Graph classes from python-type-stubs and address a few recent issues #14597
+64
−38
Conversation
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
This comment has been minimized.
This comment has been minimized.
charmoniumQ
reviewed
Aug 20, 2025
Avasam
commented
Aug 20, 2025
Comment on lines
+88
to
+93
| # Overriden in __init__ to always raise | ||
| def add_edge(self, u_of_edge: _Node, v_of_edge: _Node, **attr: Unused) -> NoReturn: ... | ||
| def add_edges_from(self, ebunch_to_add: Iterable[_EdgePlus[_Node]], **attr: Unused) -> NoReturn: ... | ||
| def add_weighted_edges_from( | ||
| self, ebunch_to_add: Iterable[tuple[_Node, _Node, float]], weight: str = "weight", **attr: Unused | ||
| ) -> NoReturn: ... |
There was a problem hiding this comment.
This isn't strictly necessary and I don't mind removing it from the PR if unwanted
This comment has been minimized.
This comment has been minimized.
…networkx-Graph-classes-from-python-type-stubs
Avasam
commented
Aug 20, 2025
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Avasam
commented
Aug 20, 2025
hamdanal
reviewed
Aug 20, 2025
charmoniumQ
approved these changes
Aug 20, 2025
This was referenced Aug 23, 2025
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Extracted |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
srittau
approved these changes
Nov 3, 2025
Avasam
deleted the
merge-networkx-Graph-classes-from-python-type-stubs
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Extracted from #14038 with a few additional changes. Should reduce changes in that PR
Changes:
Removed
intfrom degree-related returns (fixes networkx: Types for Graph properties are more general than required #14592 )Added back Liskov's Substitution Principle considerations from before complete networkx/digraph.pyi addresses #14499 follows #14509 #14569 and Complete networkx/graph.pyi. #14499 #14509
Moved comments above their related location, rather than under (pretty sure we have an unwritten convention of placing comments on the same line or above)
Removed redundant comments that just spell the annotated return type
Ordered functions to consistently match runtime
Graphs were inconsistently (as in it was only done sometime) overriding Graph methods with the same signature as their parents. I went with the following consistent approach (I'm fine switching to always overriding methods if present on the specific subclass at runtime, as long as we're consistent here)
Edit: From @Akuli : What tools and process should I use? #14499 (comment)
So we seem to be on the same track here.
There seems to have been a bit on confusion regarding these stubs recently, let's take the time to properly review these changes, and discuss any uncertainty (whilst I do think this PR is correct, I could be wrong)
CC @srittau @TomerGodinger