-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add stubs for importlib.(resources.)simple
#10931
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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -4,6 +4,7 @@ | |||
|
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.
Not a matter for this PR, but at runtime (at least on main) this file re-exports code from importlib.resources.readers
, so ideally we should do the same in our stub.
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.
Yeah, see the comments on lines 1-3 of this file :)
def open(self, *args: Never, **kwargs: Never) -> NoReturn: ... # type: ignore[override] | ||
def joinpath(self, *descendants: str) -> Traversable: ... | ||
|
||
class TraversableReader(TraversableResources, SimpleReader, metaclass=abc.ABCMeta): |
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.
Why do we need the metaclass? Doesn't seem to exist at runtime.
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.
It inherits from a class that has abstract methods, but does not implement those abstract methods, meaning it is still abstract. Because it's really easy to do that accidentally in a stub, mypy has an undocumented feature that says you have to explicitly redeclare the metaclass if that's really what you want: https://github.com/python/mypy/blob/9011ca8b4dedc0e7177737b5265f69694afa91b5/mypy/semanal_classprop.py#L94
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.
Oh yes, I implemented that feature :)
Aha, I see pre-commit is now doing the same thing to me here that it's doing to @hamdanal in #10721 🙃 See #10939 (review) for explanation... |
This comment has been minimized.
This comment has been minimized.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
And do some minor cleanup of
importlib.(resources.)readers
following 1c184fe