-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
implement PEP 585 Generic Builtins (Partially solves #7907) #9564
Conversation
This reverts commit 22a4789.
…ython version (for pep585)
@AllanDaemon I've written some test cases. Take a look at AllanDaemon#1 |
This reverts commit 22a4789.
…ython version (for pep585)
Added testcases for pep585ga
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.
Two quick comments:
- Could you also gate the GENERIC_STUB_NOT_AT_RUNTIME_TYPES in get_omitted_any behind a python version check?
- Could you add some tests for tuple? It's a bit of a special snowflake and I'm concerned it won't work
Co-authored-by: cdce8p <[email protected]>
Thanks. I wrote a few tests but I got stuck with the issue that the |
IDK. I'm not familiar yet with the code, but as far as related with PEP 585, those 2 classes are not mentioned there. I also couldn't find a code that triggers this. It this should be gated by python version, I think it would be better to open an new issue / PR.
I will. |
By now, just the test that handles the So, for this PR, after I disable the tests that fail because of So, is there anything else that should be handled before it could be merged? |
Co-authored-by: cdce8p <[email protected]>
So, what else is missing for this PR that I need to do? |
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.
This looks good, thank you for implementing PEP 585 support! I just had one minor change but otherwise I think this is good to land.
Co-authored-by: Ethan Smith <[email protected]>
Any updates? |
Thanks for your work on this! Took the liberty of tidying up / merging some tests. I'm also closing #7907. We can open a more specific issue for the low priority additions of deprecation warnings / making reveal_type reveal collections classes. We already have #9522 for the higher priority tuple reveal type issue. |
Hey team- just upgraded to T = list[int]
Let me know if you have trouble reproducing the error and/or if there's anything I can do to help! |
Yup. Sorry, I should have noted that I'm using |
The future import doesn't make a difference to type aliases, because that code is still evaluated at runtime. |
Ahh... Okay, I'm following now. That would explain why we don't want that code to type-check. Thanks for the clarification! |
Also seeing this on |
@rben01 Please read the comments above. The PEP 585 syntax in Type Aliases requires at least Python 3.9. Regardless if Once mypy supports PEP 613 you should however be able to declare them as strings in versions prior to 3.9. T: TypeAlias = "list[int]" |
@cdce8p Sorry for skipping over the previous comments. But after (re-)reading all the prior material I'm still confused. Pep 585 says this:
I think that that Pep is saying that using I tested this in an interpreter and it seems like this is correct: Text version
Python's type system is evolving way too quickly for me to keep up, and the subtle behavior differences between Python versions are a lot to keep track of so I'll defer to the Mypy team. But this was my understanding of Pep 585: that |
@rben01 That's right; I think the part that you're misunderstanding is that |
@JelleZijlstra Ok, sorry for the confusion. I saw the error message from __future__ import annotations
x: list[int] = [1] I think this is definitely a bug with mypy? I can open a new issue if there isn't one for this specific case. |
That should be allowed, yes (assuming mypy is running at least |
Ah, sorry, the underlying issue was with having two versions of mypy installed and Visual Studio Code not using the right one when linting. Ignore me 🤦 . |
Description
Partially Solves #7907
Instead of using the nongen_builtins dictionary with the non generic builtins names, it uses a function to get it that takes the python version. It returns an empty dict if the version is 3.9 or greater.
I'm opening the PR to review the code and the way it solves. Some discussion was made in the #7907 thread. I'll add the new tests to the code.
Test Plan
I'm writing some tests for this, taking in consideration the different versions. But I'm having some difficulties to get the testing stuff right, so it may take a little bit. If wanted, you can merge this and I'll open a PR with the new tests latter.