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

Merge and update variable and params annotations from typeshed #4246

Merged

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Feb 28, 2024

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_resources

Pull Request Checklist

@Avasam Avasam force-pushed the pkg_resources-variables-params-annotations branch 5 times, most recently from c9c9332 to b0e1a1e Compare February 28, 2024 06:10
Comment on lines +133 to +118
_NestedStr = Union[str, Iterable[Union[str, Iterable["_NestedStr"]]]]
_InstallerType = Callable[["Requirement"], Optional["Distribution"]]
_PkgReqType = Union[str, "Requirement"]
_EPDistType = Union["Distribution", _PkgReqType]
_MetadataType = Optional["IResourceProvider"]
Copy link
Contributor Author

@Avasam Avasam Feb 29, 2024

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 Show resolved Hide resolved
Comment on lines 1742 to 1744
egg_name: str
egg_info: str

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
egg_name: str
egg_info: str

Actually there's no assurance that an egg is even found.

@Avasam
Copy link
Contributor Author

Avasam commented Apr 21, 2024

As draft until #4262 and #4267 are completed

@Avasam Avasam requested a review from jaraco April 21, 2024 20:21
@Avasam Avasam marked this pull request as ready for review May 7, 2024 19:12
@Avasam Avasam force-pushed the pkg_resources-variables-params-annotations branch 4 times, most recently from 595bda1 to 092d919 Compare May 9, 2024 17:15
_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
Copy link
Contributor

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

Copy link
Contributor Author

@Avasam Avasam May 9, 2024

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.

Copy link
Contributor

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!

Copy link
Contributor Author

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/extern/__init__.py Outdated Show resolved Hide resolved
@@ -99,7 +123,20 @@
stacklevel=2,
)

if TYPE_CHECKING:
from _typeshed import StrPath
from types import _LoaderProtocol
Copy link
Contributor

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

Copy link
Contributor Author

@Avasam Avasam May 9, 2024

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

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -353,7 +390,7 @@ class UnknownExtra(ResolutionError):
"""Distribution doesn't have an "extra feature" of the given name"""


_provider_factories = {}
_provider_factories: Dict[Type[_ModuleLike], _AdapterType] = {}
Copy link
Contributor

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?

Copy link
Contributor Author

@Avasam Avasam May 9, 2024

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] = {}

``

@@ -363,7 +400,9 @@ class UnknownExtra(ResolutionError):
DEVELOP_DIST = -1


def register_loader_type(loader_type, provider_factory):
def register_loader_type(
loader_type: Type[_ModuleLike], provider_factory: _AdapterType
Copy link
Contributor

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?

Copy link
Contributor Author

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

@@ -2023,7 +2114,7 @@ def __init__(self, importer):
] = _declare_state('dict', '_distribution_finders', {})


def register_finder(importer_type, distribution_finder):
def register_finder(importer_type: type, distribution_finder: _AdapterType):
Copy link
Contributor

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?

Copy link
Contributor Author

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:

@@ -2201,7 +2296,7 @@ def resolve_egg_link(path):
)


def register_namespace_handler(importer_type, namespace_handler):
def register_namespace_handler(importer_type: type, namespace_handler: _AdapterType):
Copy link
Contributor

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 packages

wouldn't it?

Copy link
Contributor Author

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

@@ -3183,7 +3333,7 @@ def _always_object(classes):
return classes


def _find_adapter(registry, ob):
def _find_adapter(registry: Mapping[type, _AdapterType], ob: object):
Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Avasam Avasam force-pushed the pkg_resources-variables-params-annotations branch from ac1950e to adc40f2 Compare May 13, 2024 16:40
@Avasam
Copy link
Contributor Author

Avasam commented May 18, 2024

@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. type-ignore can be removed when updating to mypy 1.11

@Avasam Avasam requested a review from abravalheri May 18, 2024 21:21
@abravalheri abravalheri merged commit 131b6e9 into pypa:main May 22, 2024
20 of 22 checks passed
@abravalheri
Copy link
Contributor

Thank you very much.

@Avasam Avasam deleted the pkg_resources-variables-params-annotations branch May 22, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants