Don't ignore missing stubs in setuptools by Avasam · Pull Request #10058 · python/typeshed

Avasam · 2023-04-17T21:53:37Z

Given the push for the deprecation of distutils, it'd be a good idea indeed to ensure that all stubs in setuptools are accounted for.

Fixed stubtest_allowlist entries
Removed ignore_missing_stub = true
Added missing __all__
Added new default values
Standardized imports (there was a mix of relative and absolute)

…ignore-missing-stubs-in-setuptools

JelleZijlstra · 2023-04-26T20:40:15Z

+ def __enter__(self) -> None: ...
+ def __exit__(self, _exc_type, _exc_value, _traceback) -> None: ...


These are unannotated, but I don't think that should imply None since the body is just ....

Is it fine if all setuptool classes that implement the EditableStrategy protocol have the same return type for their methods? (None for __call__ and __exit__, Self for __enter__)

Maybe for __exit__, but it feels too restrictive for __enter__.

JelleZijlstra · 2023-04-26T20:40:43Z

+ path_entries: Incomplete
+ def __init__(self, dist: Distribution, name: str, path_entries: list[Path]) -> None: ...
+ def __call__(self, wheel: _WheelFile, files: list[str], mapping: dict[str, str]): ...
+ def __enter__(self): ...


Suggested change
def__enter__(self): ...
def__enter__(self)->Self: ...
(Plus import)

JelleZijlstra · 2023-04-26T20:41:07Z

+ build_lib: Incomplete
+ def __init__(self, dist: Distribution, name: str, auxiliary_dir: _Path, build_lib: _Path) -> None: ...
+ def __call__(self, wheel: _WheelFile, files: list[str], mapping: dict[str, str]): ...
+ def __enter__(self): ...


Suggested change
def__enter__(self): ...
def__enter__(self)->Self: ...

JelleZijlstra · 2023-04-26T21:12:00Z

+
+__all__ = ["Require", "find_module", "get_module_constant", "extract_constant"]
+
+def find_module(module, paths=None) -> tuple[IO[Any], str, tuple[str, Literal["", "r", "rb"], Literal[7, 6, 1, 2, 3, -1]]]: ...


Suggested change
deffind_module(module, paths=None) ->tuple[IO[Any], str, tuple[str, Literal["", "r", "rb"], Literal[7, 6, 1, 2, 3, -1]]]: ...
deffind_module(module, paths=None) ->tuple[IO[Any], str|None, tuple[str, Literal["", "r", "rb"], Literal[7, 6, 1, 2, 3, -1]]]: ...
I see some paths where it's None.

AlexWaygood

I didn't finish looking at this, but here's a partial review I started -- mostly a few suggestions for some type annotations that look fairly obvious :)

AlexWaygood · 2023-04-17T23:22:11Z

+ requirements: Iterable[Requirement],
+ env: Environment | None = None,
+ installer: _InstallerType | None = None,
+ replace_conflicting=False,


Suggested change
replace_conflicting=False,
replace_conflicting: bool=False,

AlexWaygood · 2023-04-17T23:22:23Z

 ) -> list[Distribution]: ...
 def add(self, dist: Distribution, entry: str | None = None, insert: bool = True, replace: bool = False) -> None: ...
- def subscribe(self, callback: Callable[[Distribution], object]) -> None: ...
+ def subscribe(self, callback: Callable[[Distribution], object], existing=True) -> None: ...


Suggested change
defsubscribe(self, callback: Callable[[Distribution], object], existing=True) ->None: ...
defsubscribe(self, callback: Callable[[Distribution], object], existing: bool=True) ->None: ...

AlexWaygood · 2023-04-17T23:28:59Z

 # TODO: change this to packaging.markers.Marker | None once we can import
 # packaging.markers
 marker: Incomplete | None
+ def __init__(self, requirement_string) -> None: ...


It'll be the same as https://github.com/pypa/packaging/blob/55584fb5ca327ba38b74ca5c668125caaebd9a5d/src/packaging/requirements.py#L33
Suggested change
def__init__(self, requirement_string) ->None: ...
def__init__(self, requirement_string: str) ->None: ...

AlexWaygood · 2023-04-17T23:31:53Z

+ def __lt__(self, other) -> bool: ...
+ def __le__(self, other) -> bool: ...
+ def __gt__(self, other) -> bool: ...
+ def __ge__(self, other) -> bool: ...
+ def __eq__(self, other) -> bool: ...
+ def __ne__(self, other) -> bool: ...


Suggested change
def__lt__(self, other) ->bool: ...
def__le__(self, other) ->bool: ...
def__gt__(self, other) ->bool: ...
def__ge__(self, other) ->bool: ...
def__eq__(self, other) ->bool: ...
def__ne__(self, other) ->bool: ...
def__lt__(self, other: Distribution) ->bool: ...
def__le__(self, other: Distribution) ->bool: ...
def__gt__(self, other: Distribution) ->bool: ...
def__ge__(self, other: Distribution) ->bool: ...
def__eq__(self, other: object) ->bool: ...
def__ne__(self, other: object) ->bool: ...

AlexWaygood · 2023-04-17T23:32:17Z

 @classmethod
 def from_filename(cls, filename: str, metadata: _MetadataType = None, **kw: str | None | int) -> Distribution: ...
- def activate(self, path: list[str] | None = None) -> None: ...
+ def activate(self, path: list[str] | None = None, replace=False) -> None: ...


Suggested change
defactivate(self, path: list[str] |None=None, replace=False) ->None: ...
defactivate(self, path: list[str] |None=None, replace: bool=False) ->None: ...

AlexWaygood · 2023-04-17T23:33:55Z

-
-class IResourceManager:
+@type_check_only
+class IResourceManager(Protocol):


If this class doesn't exist at runtime, should it maybe be private?
Suggested change
classIResourceManager(Protocol):
class_IResourceManager(Protocol):

Isn't that what @type_check_only is for? Granted support is still limited in mypy (python/mypy#15146, python/mypy#9531)
The source docstrings do call it IResourceManager, so it's the right name, makes it easier to find and import, whilst being safer with checkers that support the annotation.

True, mypy has been pretty slow to implement this feature :)
Elsewhere in typeshed, though, we generally don't rely on a feature unless it's supported by all major type checkers. We use @type_check_only in a few places already, but those are basically all very special cases (builtins.pyi, typing.pyi) where our standard option of making the name private isn't available.

…ignore-missing-stubs-in-setuptools

Avasam · 2023-04-29T16:53:49Z

Oh neat, you can request review from more than one person now on GitHub.

Avasam · 2023-04-29T17:10:24Z

Pretty sure pyright is failing here because types-setuptools is installed (https://github.com/python/typeshed/actions/runs/4839817600/jobs/8625083762?pr=10058#step:4:55). (notice there's no error for FileMetadata, PathMetadata and EggMetadata). Same errors locally with types-setuptools installed.

AlexWaygood · 2023-05-01T18:11:07Z

Pretty sure pyright is failing here because types-setuptools is installed (https://github.com/python/typeshed/actions/runs/4839817600/jobs/8625083762?pr=10058#step:4:55). (notice there's no error for FileMetadata, PathMetadata and EggMetadata). Same errors locally with types-setuptools installed.

Seems your diagnosis was spot on!

github-actions · 2023-05-08T16:28:56Z

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

JelleZijlstra · 2023-05-19T00:02:39Z

Looks good, @AlexWaygood any further feedback?

@Avasam feel free to mark conversations as "resolved" if you have addressed them.

AlexWaygood

Haven't reviewed in depth, but all my previous points have been addressed, nothing stands out as weird from a skim-read, and I trust @JelleZijlstra to do a thorough review :)

AlexWaygood

Haven't reviewed in depth, but all my previous points have been addressed, nothing stands out as weird from a skim-read, and I trust @JelleZijlstra to do a thorough review :)

Avasam added 3 commits April 17, 2023 17:47

Don't ignore missing stubs in setuptools
ec253ca

Merge branch 'main' ofhttps://github.com/python/typeshedinto Don't-…
b9495d8
…ignore-missing-stubs-in-setuptools

remove version dependant default
e72ce69