Skip to content

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

Merged
merged 3 commits into from
Feb 12, 2017

Conversation

elazarg
Copy link
Contributor

@elazarg elazarg commented Feb 8, 2017

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.

@gvanrossum
Copy link
Member

Nice. Can you also add the removal of those parts of #2812 that are no longer needed? (But keep the tests, obviously.)

@elazarg
Copy link
Contributor Author

elazarg commented Feb 8, 2017

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?

@elazarg
Copy link
Contributor Author

elazarg commented Feb 9, 2017

Perhaps it'd be better to do it in another PR.
(For now I am skipping the tests for enum, Just to get the green light back :) )

@gvanrossum
Copy link
Member

Seeing all that, it may be better to split this out:

  1. Reduce PR to just your initial commit
  2. Update typeshed with an EnumMeta metaclass that supports __getitem__ (it won't be activated yet because the code from Allow E[<str>] where E is an Enum type #2812 overrides it)
  3. Roll back Allow E[<str>] where E is an Enum type #2812 (but keeping its tests)

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?

@gvanrossum
Copy link
Member

Regarding the poorer error message: it would be so much better if the way types are rendered distinguished between Type[X] and X. Currently it seems Type[int] and int are both rendered as Value of type "int".

@elazarg
Copy link
Contributor Author

elazarg commented Feb 11, 2017

(Sorry I rewrote history. Another git accident there)

@gvanrossum
Copy link
Member

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?

@elazarg
Copy link
Contributor Author

elazarg commented Feb 11, 2017

Type[...] will be printed as "A value of type Type[...]".
The TypeVar case seems not to be handled at all - it evaluated to Any silently. Currently it is ignored in semanal.py, but handling it there does not fix the problem.

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).

@gvanrossum gvanrossum merged commit e0e9c2e into python:master Feb 12, 2017
@gvanrossum
Copy link
Member

gvanrossum commented Feb 12, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants