-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Use protocols instead of importlib.abc.Loader/MetaPathFinder/PathEntryFinder
#11890
Changes from all commits
c0324ae
ae8ee9d
a25120b
dde456e
a189e29
6cddd52
45b7a5c
7075e5e
eecac02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# Implicit protocols used in importlib. | ||
# We intentionally omit deprecated and optional methods. | ||
|
||
from collections.abc import Sequence | ||
from importlib.machinery import ModuleSpec | ||
from types import ModuleType | ||
from typing import Protocol | ||
|
||
__all__ = ["LoaderProtocol", "MetaPathFinderProtocol", "PathEntryFinderProtocol"] | ||
|
||
class LoaderProtocol(Protocol): | ||
def load_module(self, fullname: str, /) -> ModuleType: ... | ||
|
||
class MetaPathFinderProtocol(Protocol): | ||
def find_spec(self, fullname: str, path: Sequence[str] | None, target: ModuleType | None = ..., /) -> ModuleSpec | None: ... | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure about making it positional only arguments... but I copied the definitions from Maybe it is better just to remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protocols should usually have positional-only parameters, because otherwise implementations need to use the exact same parameter names. Use positional-or-keyword parameters only if consumers of the protocol pass keyword arguments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see, that makes a lot of sense! Thank you very much for the clarification. In accordance to that, I also introduced 6cddd52 to make the arguments in |
||
|
||
class PathEntryFinderProtocol(Protocol): | ||
def find_spec(self, fullname: str, target: ModuleType | None = ..., /) -> ModuleSpec | None: ... |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import types | ||
import zipimport | ||
from _typeshed import Incomplete, StrPath, Unused | ||
from _typeshed.importlib import LoaderProtocol | ||
from collections.abc import Callable, Generator, Iterable, Iterator, Sequence | ||
from io import BytesIO | ||
from itertools import chain | ||
|
@@ -359,7 +360,7 @@ def evaluate_marker(text: str, extra: Incomplete | None = None) -> bool: ... | |
class NullProvider: | ||
egg_name: str | None | ||
egg_info: str | None | ||
loader: types._LoaderProtocol | None | ||
loader: LoaderProtocol | None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @abravalheri Sorry I just noticed. But you won't be able to use new typeshed symbols in 3rd-party stubs in the same PR. Until the next mypy & pyright release, as they'll need to first include the new stdlib changes from typeshed. Since this is the only 3rd party stub affected, you can simply duplicate the protocol here for now. pyright is released quite often. So realistically this just means waiting for mypy for a follow-up PR. I can open a PR to update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, wasn't this part the suggestion in #11890 (comment)? (or at least that is how I interpreted the comment and then I went ahead to implement the change as a way of addressing it) If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this documented in the CONTRIBUTING guide? Sorry I might have missed that part. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can replace all usages of
When I made that comment, I didn't think about the usage in setuptools stubs. Since it's only used there and in Sorry for the "flip-flopping" here ^^"
I don't think so, but it probably should, now that you mention it. It's rare, but we've been bitten by it twice (that I can remember) in the past. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't look like this comment from Avasam was addressed before the PR was merged — I think our setuptools stubs are now importing a protocol from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
module_path: str | None | ||
|
||
def __init__(self, module: _ModuleLike) -> None: ... | ||
|
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.
You should be able to replace
types._LoaderProtocol
with this as well.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.
Replacing that in
types.pyi
would create an import loop betweentypes.pyi
and_typeshed/importlib.pyi
. Is that OK to do for type stubs?I can do the replacement in the
pkg_resources
stub without problems.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.
Loops are not a problem in stubs.
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.
Thank you for the clarification, I introduced this change in 7075e5e.