bpo-40336: Refactor typing._SpecialForm - #19620
Conversation
Sorry, something went wrong.
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks, just couple optional suggestions (up to you).
Sorry, something went wrong.
| isinstance(args[0], str) and | ||
| isinstance(args[1], tuple)): | ||
| # Close enough. | ||
| raise TypeError(f"Cannot subclass {cls!r}") |
There was a problem hiding this comment.
It would be good to keep a custom error message, but not important.
Sorry, something went wrong.
There was a problem hiding this comment.
You are right, I tested again class A(Any): pass, and got "__init__() takes 2 positional arguments but 4 were given" which does not look user friendly.
But the former message was "Cannot subclass <class 'typing._SpecialForm'>" which does not look correct, because we subclass an instance of _SpecialForm, not _SpecialForm itself. It is not possible to fix in __new__, but we can raise better error in custom __mro_entries__.
Sorry, something went wrong.
There was a problem hiding this comment.
Using __mro_entries__ is a great idea!
Sorry, something went wrong.
| return self._name | ||
|
|
||
| def __call__(self, *args, **kwds): | ||
| raise TypeError(f"Cannot instantiate {self!r}") |
There was a problem hiding this comment.
Again, a custom exception message may be helpful, but not important.
Sorry, something went wrong.
There was a problem hiding this comment.
Your are right. "Cannot instantiate typing.Any" looks better than "'_SpecialForm' object is not callable". Initially I renamed _SpecialForm to special form, but later reverted this change.
Sorry, something went wrong.
There was a problem hiding this comment.
Perfect! Thanks for fixing this.
Sorry, something went wrong.
| # There is no '_type_check' call because arguments to Literal[...] are | ||
| # values, not types. | ||
| return _GenericAlias(self, parameters) | ||
| raise TypeError(f"{self} is not subscriptable") |
There was a problem hiding this comment.
Splitting this is really great. I always wanted to do this, but didn't find a way to do this that wouldn't require boilerplate code, it looks like you have found such way.
Sorry, something went wrong.
|
I'll leave this to Ivan -- I am busy with the PEG parser integration. |
Sorry, something went wrong.
https://bugs.python.org/issue40336