-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
gh-104050: Argument clinic: more misc typing improvements #107264
Conversation
AlexWaygood
commented
Jul 25, 2023
•
edited by bedevere-bot
Loading
edited by bedevere-bot
- Issue: Add type annotations to clinic.py #104050
def __init__( | ||
cls, name: str, bases: tuple[type, ...], classdict: dict[str, Any] | ||
) -> None: | ||
converter_cls = cast(type["CConverter"], cls) | ||
add_c_converter(converter_cls) | ||
add_default_legacy_c_converter(converter_cls) |
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.
Mypy was complaining because our type annotation for add_c_converter
says (correctly) that the argument passed in has to be a subclass of CConverter
. But that isn't necessarily true here, since this metaclass could theoretically be used with classes other than CConverter
(it isn't, and it won't be, but mypy doesn't know that).
Ideally we could do this:
def __init__(
cls, name: str, bases: tuple[type, ...], classdict: dict[str, Any]
) -> None:
assert issubclass(cls, CConverter)
add_c_converter(cls)
add_default_legacy_c_converter(cls)
But we can't... because if cls
is CConverter
itself, then at this point, CConverter
doesn't exist in the global namespace yet! So that would fail with NameError
.
I therefore elected to use typing.cast()
to just tell mypy to go away here.
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.
Thanks for writing these excellent explanations!
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.
I'm glad they're all making sense! :D