-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Add collections._tuplegetter
support
#19479
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
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.
Thanks for working on this!
There should be no need to add a new type construct; descriptors like this can already be modeled by Instance
.
I think it would be sufficient to make the field types instances of a collections._tuplegetter
descriptor. Then mypy's existing descriptor handling will take care of the rest. The caveat is that collections._tuplegetter
doesn't exist in the typeshed stubs, so someone would need to add it first. If you'd be interested in doing that, the functools.cached_property
stubs work pretty similarly and can be used as a reference. Once collections._tuplegetter
exists, the mypy implementation should be as simple as using self.api.named_type("collections._tuplegetter", [typ])
for the type.
in favour of `api.named_type("collections._tuplegetter")`
This comment has been minimized.
This comment has been minimized.
in _tuplegetter descriptor
This comment has been minimized.
This comment has been minimized.
test-data/unit/check-basic.test
Outdated
[builtins fixtures/dict.pyi] | ||
[builtins fixtures/tuple.pyi] |
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.
Instead of replacing these fixtures and expanding fixtures/tuple.pyi
, I'd suggest leaving them as-is and adding import collections
to the fixtures that are used in named tuple tests. That should help reduce the diff
class _tuplegetter(Generic[KT]): | ||
if sys.version_info >= (3, 10): | ||
@overload | ||
def __get__(self, instance: None, owner: type[Any] | None = None) -> Self: ... | ||
@overload | ||
def __get__(self, instance: object, owner: type[Any] | None = None) -> KT: ... | ||
else: | ||
@overload | ||
def __get__(self, instance: None, owner: type[Any] | None) -> Self: ... | ||
@overload | ||
def __get__(self, instance: object, owner: type[Any] | None) -> KT: ... |
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.
We don't need to be too detailed for the fixture. This should suffice, and will avoid the issues with introducing sys
and Self
:
class _tuplegetter(Generic[T]):
@overload
def __get__(self, instance: None, owner: type[Any]) -> _tuplegetter[T]: ...
@overload
def __get__(self, instance: object, owner: type[Any]) -> T: ...
mypy/test/teststubtest.py
Outdated
@@ -2097,7 +2097,7 @@ class Y(TypedDict): | |||
@collect_cases | |||
def test_named_tuple(self) -> Iterator[Case]: | |||
yield Case( | |||
stub="from typing import NamedTuple", | |||
stub="from typing import NamedTuple; import collections", |
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.
Instead, let's add import collections
to stubtest_typing_stub
There are some fixtures which break But as you want to reduce diff 4 different solutions appear:
I hope this would be an acceptable compromise |
This comment has been minimized.
This comment has been minimized.
Thanks, that's a much smaller diff! Your approach for the fixtures looks good. The important thing is that it's easy for a maintainer to see how the tests have changed. A smaller diff and less changes in the actual To get the CI to run, you could try applying python/typeshed#14436 to mypy's vendored copy of typeshed at (also, can you update the PR title?) |
For the mypyc test failures, try adding |
This comment has been minimized.
This comment has been minimized.
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Fixes #19414
AddTupleGetterType
to describeNamedTuple
's class variablesI could get away with someNotImplementedError
's, and this is most likely because tests are not exhaustive, but I have no clue which use cases will trigger those methods