Skip to content
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

bpo-40336: Refactor typing._SpecialForm #19620

Merged

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Apr 20, 2020

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, just couple optional suggestions (up to you).

isinstance(args[0], str) and
isinstance(args[1], tuple)):
# Close enough.
raise TypeError(f"Cannot subclass {cls!r}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to keep a custom error message, but not important.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using __mro_entries__ is a great idea!

Lib/typing.py Outdated

def __repr__(self):
return 'typing.' + self._name

def __reduce__(self):
return self._name

def __call__(self, *args, **kwds):
raise TypeError(f"Cannot instantiate {self!r}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, a custom exception message may be helpful, but not important.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! Thanks for fixing this.

# 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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gvanrossum
Copy link
Member

I'll leave this to Ivan -- I am busy with the PEG parser integration.

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

Successfully merging this pull request may close these issues.

None yet

6 participants