-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Allow using __getitem__ defined in metaclasses of non-generic classes #2827
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
Conversation
Nice. Can you also add the removal of those parts of #2812 that are no longer needed? (But keep the tests, obviously.) |
The problematic part in this PR is the reduced clarity in error messages, as can be seen in the (currently failing) tests. What do you think about it? |
Perhaps it'd be better to do it in another PR. |
Seeing all that, it may be better to split this out:
You should be able to co-develop (2) and (3) and submit them concurrently, with a dependency on merging so that (2) gets merged first but only once you know that the tests for (3) pass with that included. The PR for (3) should be updated at the last moment with a typeshed sync that incorporates (2). Does that make sense? |
Regarding the poorer error message: it would be so much better if the way types are rendered distinguished between |
42b63c5
to
7abf8f4
Compare
(Sorry I rewrote history. Another git accident there) |
What if the indexed class is e.g. Type[...], Or a type var bounded by such a thing? Or has a bound that is a metaclass? |
I am a bit hesitant about changing the pretty-printing since it might affect many tests. (I am sorry but I do not have enough time right now, and I will not be available at all in the upcoming week). |
OK, will merge as-is.
|
Fix #1771. (Seems to be much simpler than I thought)
In particular, this will allow removing some special-casing for enum (#2812).
Related to #1381.