-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add annotations for MultiDiGraph
#13319
base: main
Are you sure you want to change the base?
Conversation
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.
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. 🤖🎉 |
self, nbunch: None = None, data: Literal[False] = False, *, default: Unused = None, keys: Literal[True] | ||
) -> Self: ... | ||
@overload | ||
def __call__( | ||
self, nbunch: _NBunch[_Node], data: Literal[True], *, default: None = None, keys: bool = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we rearrange the overloads so that the true/false pairs are next to each other so it's easier to read?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong here, but it appears this PR splits some bool
overloads into two separate overloads that take True
and False
. I'm wondering if we could arrange the order of the overload declarations so that if the original ordering was
@overload
overload1
@overload
overload2
It would now be
@overload
overload1True
@overload
overload1False
@overload
overload2True
@overload
overload2False
etc.
That would make it a bit easier to review and probably also easier to edit/maintain in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the order of OutEdgeView
for nbunch
, data
, etc., and so changing the order would make it inconsistent with the rest of the file.
@@ -13,6 +13,8 @@ class MultiDiGraph(MultiGraph[_Node], DiGraph[_Node]): | |||
@cached_property | |||
def pred(self) -> MultiAdjacencyView[_Node, _Node, dict[str, Incomplete]]: ... | |||
@cached_property | |||
def edges(self) -> OutMultiEdgeView[_Node]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part lgtm
A few overloads for edge views that include keys were missing, as well as
MultiDiGraph.edges
.