-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 support for pep585 with postponed evaluation #4059
Add support for pep585 with postponed evaluation #4059
Conversation
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.
Thank you for the MR, very clean. This change look reasonable, would it be possible maybe to check that from __future__ import annotations
is present if we're under python 3.9 ? Ie. The same functional file without from __future__ import annotations
would raise Error for python under 3.9 ? Also sorry for the master branch failing that also makes your CI fail, no idea how that happened, maybe a flaky test.
@Pierre-Sassoulas Thanks for the quick review. Regarding your comments
I believe it already does. I've added
I noticed that the master branch is now passing, I will rebase the MR. Maybe that solves it. |
Didn't work. I was already using the latest master. Maybe it will work for the next commit. |
It worked, as expected. I've included the checks in my last commit. |
Sorry for the many updates, the MR should be ready now. |
Thanks a lot ! I'll merge this as soon as the build is fixed. Apparently the problem come from the current astroid master branch. Can you please allow edit from maintainer so I can rebase your branch myself once it's fixed ? :) |
@Pierre-Sassoulas Already checked. If it doesn't work for whatever reason, just let me know. I usually can get back to it within a day. |
@Pierre-Sassoulas I saw that there are plans to do a new release soon pylint-dev/astroid#886 (comment). |
@Pierre-Sassoulas I noticed today that I was missing checks for |
Sorry for the back and forth, I find it quite difficult to get it right with checking so many different things which aren't really documented anywhere. However I think I got it now. Turns out my last code change was wrong, with # Valid in Python 3.7/3.8 (TypedDict)
from __future__ import annotations
class CustomNamedTuple(NamedTuple):
my_var: list[int]
class CustomTypedDict(TypedDict):
my_var: list[int]
@dataclass
class CustomDataClass:
my_var: list[int] What doesn't work and I guess caused my confusion is: CustomNamedTuple = typing.NamedTuple(
"CustomNamedTuple", [("my_var", list[int])]) # [unsubscriptable-object]
CustomTypedDict = TypedDict("CustomTypedDict", my_var=list[int]) # [unsubscriptable-object]
CustomTypedDict2 = TypedDict("CustomTypedDict2", {"my_var": list[int]}) # [unsubscriptable-object] Just to be sure I also checked it with Pylance / pyright, but, as it turns out, even it doesn't recognize all cases correctly so I also did some manual checks again. |
No worry, mypy 0.8 has only been released 2 weeks ago, this is cutting edge statistical analysis on multiple python interpreters after all ;) |
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.
Great PR, thank you @cdce8p !
1 similar comment
@Pierre-Sassoulas This one is ready to be merged. |
Steps
doc/whatsnew/<current release.rst>
.Description
With PEP 585 and postponed evaluation of annotations it's possible to use
list[int]
and others even in versions3.7
and3.8
although PEP 585 was implemented in 3.9.Currently, running pylint in a 3.7 or 3.8 environment always returns
unsubscriptable-object
errors. However the following example is validThe error is correct only if the type is evaluated instantly as for example with type aliases
The latest mypy version 0.800 just added support for PEP 585 so pylint is the only thing holding it back for many.
--
Since I don't have much experience developing for pylint, some guidance would be much appreciated.
There is probably a better way to do this 😄
Type of Changes
Related Issue
Closes: #3320