-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
Process NameTuple and fix Union types using | #13788
Conversation
@@ -595,6 +606,17 @@ class X(NamedTuple): | |||
a: Incomplete | |||
b: Incomplete | |||
|
|||
[case testNamedtupleWithTypes] | |||
import collections, x | |||
X = typing.NamedTuple('X', [('a', str), ('b', str)]) |
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.
Can you please also test a case like X = typing.NamedTuple('X', [('@', 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.
@sobolevn, won't that fail at runtime?
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 will fail at runtime:
E ValueError: Type names and field names must be valid identifiers: '@'
I have added isidentifier validation.
isinstance(callee, MemberExpr) and callee.name == "namedtuple" | ||
return ( | ||
isinstance(callee, NameExpr) | ||
and (callee.name.endswith("namedtuple") or callee.name.endswith("NamedTuple")) |
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.
Would this match something like notanamedtuple
?
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.
Yes I don't know what mypy support for "namedtuple" is, but I wanted the same behaviour for Namedtuple (NameExpr).
Original code was:
return (isinstance(callee, NameExpr) and callee.name.endswith("namedtuple")) or (
isinstance(callee, MemberExpr) and callee.name == "namedtuple"
I just added NamedTuple :)
collections.namedtuple vs typing.NamedTuple.
It seems that NameExpr can be qualified name:
self.name = name # Name referred to (may be qualified)
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.
Both NameExpr
and MemberExpr
have .fullname
attribute. Maybe it is better to use it instead?
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.
Hm I tried ... but fullname is resolved only when running stubgen from cmd line but not when running unit tests (it is None) and all namedtuple tests failing. Any idea?
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.
So for fullname I need _semanal tests otherwise parseonly is enabled for unit tests. I can switch to semanal for namedtuple tests but it seems excessive. Thoughts?
isinstance(callee, MemberExpr) and callee.name == "namedtuple" | ||
return ( | ||
isinstance(callee, NameExpr) | ||
and (callee.name.endswith("namedtuple") or callee.name.endswith("NamedTuple")) |
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.
Both NameExpr
and MemberExpr
have .fullname
attribute. Maybe it is better to use it instead?
@@ -595,6 +606,33 @@ class X(NamedTuple): | |||
a: Incomplete | |||
b: Incomplete | |||
|
|||
[case testNamedtupleUsingInvalidIdent] | |||
import collections, x | |||
X = collections.namedtuple('X', ['@']) |
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 don't think that it is correct. It just drops a field that is present in the original code.
I think a proper way to fix is to use X = collections.namedtuple('X', ['@'])
in stubs.
There are several typeshed examples where we actually do this.
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.
Generating X = collections.namedtuple('X', ['@', 'a', 'b', 'c']) in the stubs would create invalid stub code.
Well imho I prefer partial correctness (and correct code), since we could resolve other fields (a, b, c in my case) and skip incorrect one.
Fixes python#9901 Fixes python#13662 Fix inheriting from a call-based `collections.namedtuple` / `typing.NamedTuple` definition that was omitted from the generated stub. This automatically adds support for the call-based `NamedTuple` in general not only as a based class (Closes python#13788). <details> <summary>An example before and after</summary> Input: ```python import collections import typing from collections import namedtuple from typing import NamedTuple CollectionsCall = namedtuple("CollectionsCall", ["x", "y"]) class CollectionsClass(namedtuple("CollectionsClass", ["x", "y"])): def f(self, a): pass class CollectionsDotClass(collections.namedtuple("CollectionsClass", ["x", "y"])): def f(self, a): pass TypingCall = NamedTuple("TypingCall", [("x", int | None), ("y", int)]) class TypingClass(NamedTuple): x: int | None y: str def f(self, a): pass class TypingClassWeird(NamedTuple("TypingClassWeird", [("x", int | None), ("y", str)])): z: float | None def f(self, a): pass class TypingDotClassWeird(typing.NamedTuple("TypingClassWeird", [("x", int | None), ("y", str)])): def f(self, a): pass ``` Output diff (before and after): ```diff diff --git a/before.pyi b/after.pyi index c88530e2c..95ef843b4 100644 --- a/before.pyi +++ b/after.pyi @@ -1,26 +1,29 @@ +import typing from _typeshed import Incomplete from typing_extensions import NamedTuple class CollectionsCall(NamedTuple): x: Incomplete y: Incomplete -class CollectionsClass: +class CollectionsClass(NamedTuple('CollectionsClass', [('x', Incomplete), ('y', Incomplete)])): def f(self, a) -> None: ... -class CollectionsDotClass: +class CollectionsDotClass(NamedTuple('CollectionsClass', [('x', Incomplete), ('y', Incomplete)])): def f(self, a) -> None: ... -TypingCall: Incomplete +class TypingCall(NamedTuple): + x: int | None + y: int class TypingClass(NamedTuple): x: int | None y: str def f(self, a) -> None: ... -class TypingClassWeird: +class TypingClassWeird(NamedTuple('TypingClassWeird', [('x', int | None), ('y', str)])): z: float | None def f(self, a) -> None: ... -class TypingDotClassWeird: +class TypingDotClassWeird(typing.NamedTuple('TypingClassWeird', [('x', int | None), ('y', str)])): def f(self, a) -> None: ... ``` </details>
from
to
and small fix for: #12929 fixing missing Union import