-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Merge and update variable and params annotations from typeshed #4246
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
Merge and update variable and params annotations from typeshed #4246
Conversation
c9c9332 to
b0e1a1e
Compare
| _NestedStr = Union[str, Iterable[Union[str, Iterable["_NestedStr"]]]] | ||
| _InstallerType = Callable[["Requirement"], Optional["Distribution"]] | ||
| _PkgReqType = Union[str, "Requirement"] | ||
| _EPDistType = Union["Distribution", _PkgReqType] | ||
| _MetadataType = Optional["IResourceProvider"] |
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.
These aliases come from the typeshed stub. Please lmk if you'd prefer different names or to make them public.
pkg_resources/__init__.py
Outdated
| egg_name: str | ||
| egg_info: str | ||
|
|
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.
| egg_name: str | |
| egg_info: str |
Actually there's no assurance that an egg is even found.
595bda1 to
092d919
Compare
| _MetadataType = Optional["IResourceProvider"] | ||
| # Any object works, but let's indicate we expect something like a module (optionally has __loader__ or __file__) | ||
| _ModuleLike = Union[object, types.ModuleType] | ||
| _AdapterType = Callable[..., Any] # Incomplete |
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 very much Avasam for having a look on this.
I believe that instead of _AdapterType it is better to define something like _DistributionFinder, _NamespaceHandler, _ProviderFactory ...
Then _find_adapter can possibly be defined using typing.overload for the special cases (untested).
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 plan on updating with:
_ProviderFactoryType = Callable[[_ModuleLike], "IResourceProvider"]
_DistFinderType = Callable[[_T, str, bool], Iterable["Distribution"]]
_NSHandlerType = Callable[[_T, str, str, types.ModuleType], Optional[str]]
_AdapterT = TypeVar(
"_AdapterT", _DistFinderType[Any], _ProviderFactoryType, _NSHandlerType[Any]
)
# ...
def _find_adapter(
registry: Mapping[type, _AdapterT],
ob: object,
) -> _AdapterT: ...in a follow-up PR, but I can add it to this one if you'd like.
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.
If you are planning that as a follow-up PR, that should be fine.
Thank you very much!
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've already created the follow-up here: 2c2874d (#4355)
pkg_resources/__init__.py
Outdated
|
|
||
| if TYPE_CHECKING: | ||
| from _typeshed import StrPath | ||
| from types import _LoaderProtocol |
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.
This is an import of a private member... Does _typeshed provide an equivalent? If not, we probably have to open an FR there before being able to import it 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.
That comes from typeshed. https://github.com/python/typeshed/blob/main/stdlib/types.pyi#L321C1-L322C63
It's private to help prevent accidental runtime imports because typeshed didn't start using @type_check_only until very recently (and I believe we'll keep these names private anyway). In other words, _LoaderProtocol doesn't exist in the types module.
We could potentially ask to move _LoaderProtocol from types.pyi to typeshed/__init__.pyi and see what other typeshed maintainers think.
Alternatively, I can declare the protocol here close to the other aliases
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.
If _LoaderProtocol is undocumented, I think we should not import it.
If typeshed maintainers are happy to make an utility definition of LoaderProtocol in _typeshed accessible for public usage that would be great. Otherwise if the protocol is small, declaring the protocol inline should also be fine.
(Brain dump: probably having an _typeshed.importlib module that helps to address the concerns in python/typeshed#11882 (comment) would be even greater).
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 in favor of python/typeshed#11890 (comment)
|
|
||
|
|
||
| _provider_factories = {} | ||
| _provider_factories: Dict[Type[_ModuleLike], _AdapterType] = {} |
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.
Instead of _AdapterType, it would be more "sound" here to have a callable that, passed a *module* object, returns an ``IResourceProvider`` for that module., would it?
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.
See comment https://github.com/pypa/setuptools/pull/4246/files#r1596038268
This would become
_provider_factories: Dict[Type[_ModuleLike], _ProviderFactoryType] = {}
``` |
|
||
| def register_loader_type(loader_type, provider_factory): | ||
| def register_loader_type( | ||
| loader_type: Type[_ModuleLike], provider_factory: _AdapterType |
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.
Same as previous: instead of _AdapterType, it would be more "sound" here to have a callable that, passed a *module* object, returns an ``IResourceProvider`` for that module., would it?
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.
See comment https://github.com/pypa/setuptools/pull/4246/files#r1596038268
This would become
def register_loader_type(
loader_type: Type[_ModuleLike],
provider_factory: _ProviderFactoryType,
) -> None: ...|
|
||
|
|
||
| def register_finder(importer_type, distribution_finder): | ||
| def register_finder(importer_type: type, distribution_finder: _AdapterType): |
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.
Instead of _AdapterType, it would be more "sound" here to have a callable that, passed a path item and the importer instance, yields ``Distribution`` instances found on that path item or something that matches the same type as the _distribution_finders variable above, would it?
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.
See comment https://github.com/pypa/setuptools/pull/4246/files#r1596038268
This would become:
def register_finder(
importer_type: Type[_T], distribution_finder: _DistFinderType[_T]
) -> None:|
|
||
|
|
||
| def register_namespace_handler(importer_type, namespace_handler): | ||
| def register_namespace_handler(importer_type: type, namespace_handler: _AdapterType): |
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.
Instead of _AdapterType, it would be more "sound" here to have a callable like this:
def namespace_handler(importer, path_entry, moduleName, module):
# return a path_entry to use for child packageswouldn't it?
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.
See comment https://github.com/pypa/setuptools/pull/4246/files#r1596038268
This would become:
def register_namespace_handler(
importer_type: Type[_T],
namespace_handler: _NSHandlerType[_T],
) -> None: ...|
|
||
|
|
||
| def _find_adapter(registry, ob): | ||
| def _find_adapter(registry: Mapping[type, _AdapterType], ob: object): |
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.
Here the way to go is probably have 3 definitions using typing.overload for the 3 different types of adapters and compatible ob (something like _ProviderFactory+ _LoaderProtocol; _DistributionFider+_Importer; _NamespaceHandler+_Importer, with _Importer a protocol that is compatible with PathEntryFinder and/or MetaPathFinder? ... considering the limitations in python/typeshed#11541 and python/typeshed#2468)
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.
ac1950e to
adc40f2
Compare
|
@abravalheri in-code comments have been updated to reflect that fixes have been done upstream. I don't think we need to wait for the next mypy release. |
|
Thank you very much. |
Summary of changes
Merge and update (because they were not all correct or exact) variable and params annotations from typeshed. Missing public annotations are missing from typeshed too.
This PR purposefully avoids adding TypeVars, overloads and return types. Those will all be tackled in a follow-up PR. Return types are inferred at this step.
Step 3.1 of #2345 (comment) but only for
pkg_resourcesPull Request Checklist
newsfragments/.(See documentation for details)