- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.9k
Correct type annotations on NetworkX DiGraphs #14595
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
Conversation
| Thanks, this closes #14592 | 
| Diff from mypy_primer, showing the effect of this PR on open source code: discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/commands/hybrid.py:508: error: Overlap between argument names and ** TypedDict items: "description", "name"  [misc]
+ discord/ext/commands/hybrid.py:508: error: Overlap between argument names and ** TypedDict items: "name", "description"  [misc]
- discord/ext/commands/hybrid.py:629: error: Overlap between argument names and ** TypedDict items: "description", "name"  [misc]
+ discord/ext/commands/hybrid.py:629: error: Overlap between argument names and ** TypedDict items: "name", "description"  [misc]
 | 
| def __call__(self, data: Literal[False] = False, default=None) -> Self: ... | ||
| @overload | ||
| def __call__(self, data: Literal[True] | str, default=None) -> NodeDataView[_Node]: ... | ||
| def data(self, data: bool | str = True, default=None) -> NodeDataView[_Node]: ... | ||
| def __call__(self, data: Literal[True] | str, default=None) -> Self: ... | ||
| def data(self, data: bool | str = True, default=None) -> Self: ... | 
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 believe @srittau 's comment in #14595 (comment) only applied to the first overload
When data is True, it's not returning self

As-is, this overload is useless since only the params differ (could be a Union, but I think it was correct to have an overload, see:)
| def __call__(self, data: Literal[False] = False, default=None) -> Self: ... | |
| @overload | |
| def __call__(self, data: Literal[True] | str, default=None) -> NodeDataView[_Node]: ... | |
| def data(self, data: bool | str = True, default=None) -> NodeDataView[_Node]: ... | |
| def __call__(self, data: Literal[True] | str, default=None) -> Self: ... | |
| def data(self, data: bool | str = True, default=None) -> Self: ... | |
| @overload | |
| def __call__(self, data: Literal[False] = False, default=None) -> Self: ... | |
| @overload | |
| def __call__(self, data: Literal[True] | str, default=None) -> NodeDataView[_Node]: ... | |
| @overload | |
| def data(self, data: Literal[False], default=None) -> Self: ... | |
| @overload | |
| def data(self, data: Literal[True] | str = True, default=None) -> NodeDataView[_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.
You are right; I misread NodeView as NodeDataView in a haste.
| @overload | ||
| def __call__(self, nbunch: None = None, weight: None | bool | str = None) -> int: ... # type: ignore[overload-overlap] | ||
| @overload | ||
| def __call__(self, nbunch: None | Iterable[_Node], weight: None | bool | str = None) -> Self: ... | 
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.
The overlap error is correct here. I also think it's inverted since nbunch=None should return self
| @overload | |
| def __call__(self, nbunch: None = None, weight: None | bool | str = None) -> int: ... # type: ignore[overload-overlap] | |
| @overload | |
| def __call__(self, nbunch: None | Iterable[_Node], weight: None | bool | str = None) -> Self: ... | |
| @overload | |
| def __call__(self, nbunch: None = None, weight: None | bool | str = None) -> Self: ... | |
| @overload | |
| def __call__(self, nbunch: Iterable[_Node], weight: None | bool | str = None) -> int: ... | 
 
    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.
You're right; the overload should be specialized to _Node (which still needs to ignore possible overlap between None/NoneType and _Node, as is done in OutEdgeView).
None and Iterable[_Node] return Self (first and last return), but _Node returns int (middle two returns)
(ignoring, like we have been, that it could actually return float in the case of graphs whose edge weights are float)
| Oh I was too slow on the review and this got merged ^^" No worries, I'll propose the same changes in #14597 | 
Two fixes:
in_degreeis acached_propertythat isInDegreeVieworInMultiDegreeView(for aMultiDiGraph).InDegreeViewcan then return anintor itself when called. However, theGraph's propertyin_degreeis never itself anint, always anInDegreeView(source).NodeViewreturns not just anyIterator, but aNodeViewitself. See example in documentation,