Repository navigation
[networkx] Fix NodeDataView return type for string attribute in nodes(data=...) - #16695
hrithiksaini99 wants to merge 2 commits into
Conversation
…(data=...) (python#16548) - Update NodeView.__call__ and NodeView.data overloads to return NodeDataView[_Node, _U] when data is a string - Make NodeDataView generic over [_Node, _D] so __iter__ yields tuple[_Node, _D] and __getitem__ returns _D - Fixes python#16548
This comment has been minimized.
This comment has been minimized.
|
|
||
| @overload | ||
| def __call__(self, data: Literal[False] = False, default=None) -> Self: ... | ||
| def __call__(self, data: Literal[False] = False, default: Unused = None) -> Self: ... |
There was a problem hiding this comment.
I think we should try to remove the default argument for this branch to catch misuse of this API. If that doesn't work, try to use None as type argument.
There was a problem hiding this comment.
Removed the default parameter from this overload — default is only consulted when data is a string, so passing it here is misuse.
| def __call__(self, data: Literal[False] = False, default: Unused = None) -> Self: ... | ||
| @overload | ||
| def __call__(self, data: Literal[True] | str, default=None) -> NodeDataView[_Node, _NodeData, _EdgeData]: ... | ||
| def __call__(self, data: Literal[True], default: None = None) -> NodeDataView[_Node, _NodeData]: ... |
There was a problem hiding this comment.
Removed default here too; it now only exists on the data: str overload.
| def data(self, data: Literal[False], default: Unused = None) -> Self: ... | ||
| @overload | ||
| def data(self, data: Literal[True] = True, default: None = None) -> NodeDataView[_Node, _NodeData]: ... |
There was a problem hiding this comment.
Applied the same to data(): default is gone from the Literal[False] and Literal[True] overloads and only remains on the data: str one.
| def data(self, data: str, default: _U | None = None) -> NodeDataView[_Node, _U]: ... | ||
|
|
||
| class NodeDataView(AbstractSet[_Node], Generic[_Node, _NodeData, _EdgeData]): | ||
| class NodeDataView(AbstractSet[_Node], Generic[_Node, _D]): |
There was a problem hiding this comment.
NodeDataView previously always yielded tuple[_Node, _NodeData], which is wrong when data is a string — in that case the view yields the value stored under that attribute key, not the node's data mapping. So the second type parameter now describes the yielded value type (_D) rather than the node-data mapping, and the unused _EdgeData parameter was dropped. This mirrors OutEdgeDataView[_Node, _D] in the same file, which parameterises its yielded element the same way.
| class NodeDataView(AbstractSet[_Node], Generic[_Node, _D]): | ||
| __slots__ = ("_nodes", "_data", "_default") | ||
| def __init__(self, nodedict: _NodeData, data: bool | str = False, default=None) -> None: ... | ||
| def __init__(self, nodedict, data: bool | str = False, default: object = None) -> None: ... |
There was a problem hiding this comment.
Why remove the annotation for nodedict?
Why use object for default? How is it used?
There was a problem hiding this comment.
Good catch on both counts. nodedict is the graph's _node mapping (dict[_Node, _NodeData]), and since _NodeData is no longer a type parameter of this class the old annotation no longer works, which is why I had dropped it. I have now annotated it as Mapping[_Node, Mapping[str, Any]].
default is the value returned when the requested attribute key is missing from a node's data, so it can be any value — hence object. I have changed it to Any to make the intent clearer. Happy to leave both unannotated to match OutEdgeDataView.__init__ if you would prefer that.
- Drop the meaningless `default` parameter from the `data: Literal[False]` and `data: Literal[True]` overloads of `NodeView.__call__` and `NodeView.data` so that passing it is reported as misuse. `default` is only meaningful when `data` is a string. - Restore an annotation for `nodedict` in `NodeDataView.__init__` and give `default` a concrete type instead of the bare `object`.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Fixes #16548
According to the NetworkX documentation and runtime behavior for
Graph.nodes(data=...):Previously,
NodeView.__call__andNodeView.datatypeddata: Literal[True] | stras returningNodeDataView[_Node, _NodeData, _EdgeData], which always yieldedtuple[_Node, _NodeData](the full dictionary) even when an attribute string was requested.This PR:
data: Literal[True](returningNodeDataView[_Node, _NodeData]) anddata: str(returningNodeDataView[_Node, _U], generic over the extracted attribute type).NodeDataViewto beGeneric[_Node, _D], yieldingtuple[_Node, _D]on iteration and returning_Don item lookup (matchingOutEdgeDataView).