-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix mypy type checking via creating a nox typecheck session #13476
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
Changes from all commits
f22e3b7
63d785c
e8b735f
9931fb5
02c8b5c
ad9bf38
47d03cc
74acb42
ab0bcc3
7eebf11
b3002e3
914ccdb
f0fce92
f9af7ef
c0e93c1
ac3d85c
a203957
eaf7836
20c5498
49f5a13
32e4afa
cc1ad64
1aea5ba
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 |
|---|---|---|
|
|
@@ -81,6 +81,48 @@ test-common-wheels = [ | |
| "pytest-subket >= 0.8.1", | ||
| ] | ||
|
|
||
| docs = [ | ||
| "sphinx ~= 7.0", | ||
| # currently incompatible with sphinxcontrib-towncrier | ||
| # https://github.com/sphinx-contrib/sphinxcontrib-towncrier/issues/92 | ||
| "towncrier < 24", | ||
| "furo", | ||
| "myst_parser", | ||
| "sphinx-copybutton", | ||
| "sphinx-inline-tabs", | ||
| "sphinxcontrib-towncrier >= 0.2.0a0", | ||
| "sphinx-issues" | ||
| ] | ||
|
|
||
| # Libraries that are not required for pip to run, | ||
| # but are used in some optional features. | ||
| all-optional = ["keyring"] | ||
|
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. Seems very curious to me to be using dependency-groups to specify optional extras... 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. So historically pip has a couple of dependencies it has where if it is present in the environment it will behave differently, notable 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. Pip has historically had no dependencies at all. That's an explicit and deliberate design choice - pip is not a package that you install "normally"1, so it needs to be usable in a situation where no pre-existing package installer is available. Working out how to install optional dependencies while bootstrapping pip is non-trivial (and not something I'd want to support), while once pip is installed, So rather than have to deal with the support issues of all of this, we simply note that if the user has keyring installed, pip will make use of it. I don't object to having an Footnotes
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.
The usefulness here to me is for two reasons:
But this is not a sticking point for me, I'm willing to put it in the noxfile directly. |
||
|
|
||
| nox = ["nox"] # noxfile.py | ||
| update-rtd-redirects = ["httpx", "rich", "PyYAML"] # tools/update-rtd-redirects.py | ||
|
|
||
| type-checking = [ | ||
| # Actual type checker: | ||
| "mypy", | ||
|
|
||
| # Stub libraries that contain type hints as a separate package: | ||
| "types-docutils", # via sphinx (test dependency) | ||
| "types-requests", # vendored | ||
| "types-urllib3", # vendored (can be removed when we upgrade to urllib3 >= 2.0) | ||
| "types-setuptools", # test dependency and used in distutils_hack | ||
| "types-six", # via python-dateutil via freezegun (test dependency) | ||
| "types-PyYAML", # update-rtd-redirects dependency | ||
| ] | ||
|
|
||
| all = [ | ||
| {include-group = "test"}, | ||
| {include-group = "docs"}, | ||
| {include-group = "nox"}, | ||
| {include-group = "all-optional"}, | ||
| {include-group = "update-rtd-redirects"}, | ||
| {include-group = "type-checking"}, | ||
| ] | ||
|
|
||
| [tool.setuptools] | ||
| package-dir = {"" = "src"} | ||
| include-package-data = false | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,9 @@ | |
| import shutil | ||
| import site | ||
| from optparse import SUPPRESS_HELP, Values | ||
| from typing import TYPE_CHECKING | ||
|
|
||
| from pip._vendor.packaging.utils import canonicalize_name | ||
| from pip._vendor.requests.exceptions import InvalidProxyURL | ||
| from pip._vendor.rich import print_json | ||
|
|
||
| # Eagerly import self_outdated_check to avoid crashes. Otherwise, | ||
|
|
@@ -60,6 +60,12 @@ | |
| ) | ||
| from pip._internal.wheel_builder import build, should_build_for_install_command | ||
|
|
||
| if TYPE_CHECKING: | ||
| # Vendored libraries with type stubs | ||
| from requests.exceptions import InvalidProxyURL | ||
| else: | ||
| from pip._vendor.requests.exceptions import InvalidProxyURL | ||
|
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. Stylistically, I find the moving of the real imports to be uncomfortable. An alternative pattern: Alternatively, perhaps we should just vendor This latter option will be the least error-prone, as I can see that it will be easy for developers to accidentally use the real requests instead of the vendored one (e.g. in 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 guess this is a drive-by review of your drive-by review, but in general I’m -1 on vendoring things that we only require for type checking. That feels like runtime bloat for something that is only needed at development time. 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.
😂 - and it is a very welcome comment 👍
Yes, fully accept. The proposal in this PR currently adds maintenance bloat, which is arguably a more precious resource for the project, but I fully see your perspective. Given the fact that this is a developer-time requirement, then I think there is a reasonable middle-ground where you could install & vendor the typestub packages at test time, and not actually commit the vendored type libraries. 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. @philfreo I'm pretty sure your proposed pattern doesn't work for type checkers, it throws an error that I went though several rounds of how to provide the type stub hints to the the type checker without creating any run time differences and landed on the current approach as fairly simple, the most readable, and works in all situations. But I am happy to find out there's a simpler or more readable version. 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. At a high level the options here are:
And in my opinion here are the downsides:
I do not want 1 and 4, because I don't want to be running mypy producing incorrect type checking, and I don't want to affect the pip distribution with type hint stuff. I prefer 3 because I think I've found a pattern that is fairly readable and maintainable, so if 3 is rejected I would look to implement 2. |
||
|
|
||
| logger = getLogger(__name__) | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,15 +18,12 @@ | |
| from html.parser import HTMLParser | ||
| from optparse import Values | ||
| from typing import ( | ||
| TYPE_CHECKING, | ||
| Callable, | ||
| NamedTuple, | ||
| Protocol, | ||
| ) | ||
|
|
||
| from pip._vendor import requests | ||
| from pip._vendor.requests import Response | ||
| from pip._vendor.requests.exceptions import RetryError, SSLError | ||
|
|
||
| from pip._internal.exceptions import NetworkConnectionError | ||
| from pip._internal.models.link import Link | ||
| from pip._internal.models.search_scope import SearchScope | ||
|
|
@@ -38,6 +35,15 @@ | |
|
|
||
| from .sources import CandidatesFromPage, LinkSource, build_source | ||
|
|
||
| if TYPE_CHECKING: | ||
| # Vendored libraries with type stubs | ||
| import requests | ||
| from requests import Response | ||
| from requests.exceptions import RetryError, SSLError | ||
| else: | ||
| from pip._vendor import requests | ||
| from pip._vendor.requests.exceptions import RetryError, SSLError | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| ResponseHeaders = MutableMapping[str, str] | ||
|
|
@@ -80,6 +86,7 @@ def _ensure_api_header(response: Response) -> None: | |
| ): | ||
| return | ||
|
|
||
| assert response.request.method is not 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. Runtime behaviour change. I guess this is because the type hints suggest that method can be none. If that is the case, do we need to adapt the code to handle that case... ? (e.g. |
||
| raise _NotAPIContent(content_type, response.request.method) | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |
| import pathlib | ||
| import sys | ||
| import sysconfig | ||
| from typing import Any | ||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
| from pip._internal.models.scheme import SCHEME_KEYS, Scheme | ||
| from pip._internal.utils.compat import WINDOWS | ||
|
|
@@ -80,7 +80,7 @@ def _looks_like_bpo_44860() -> bool: | |
|
|
||
| See <https://bugs.python.org/issue44860>. | ||
| """ | ||
| from distutils.command.install import INSTALL_SCHEMES | ||
| from distutils.command.install import INSTALL_SCHEMES # type: ignore | ||
|
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. Suggest that |
||
|
|
||
| try: | ||
| unix_user_platlib = INSTALL_SCHEMES["unix_user"]["platlib"] | ||
|
|
@@ -105,7 +105,7 @@ def _looks_like_red_hat_lib() -> bool: | |
|
|
||
| This is the only way I can see to tell a Red Hat-patched Python. | ||
| """ | ||
| from distutils.command.install import INSTALL_SCHEMES | ||
| from distutils.command.install import INSTALL_SCHEMES # type: ignore | ||
|
|
||
| return all( | ||
| k in INSTALL_SCHEMES | ||
|
|
@@ -117,7 +117,7 @@ def _looks_like_red_hat_lib() -> bool: | |
| @functools.cache | ||
| def _looks_like_debian_scheme() -> bool: | ||
| """Debian adds two additional schemes.""" | ||
| from distutils.command.install import INSTALL_SCHEMES | ||
| from distutils.command.install import INSTALL_SCHEMES # type: ignore | ||
|
|
||
| return "deb_system" in INSTALL_SCHEMES and "unix_local" in INSTALL_SCHEMES | ||
|
|
||
|
|
@@ -131,8 +131,13 @@ def _looks_like_red_hat_scheme() -> bool: | |
| (fortunately?) done quite unconditionally, so we create a default command | ||
| object without any configuration to detect this. | ||
| """ | ||
| from distutils.command.install import install | ||
| from distutils.dist import Distribution | ||
| if TYPE_CHECKING: | ||
| # Vendored libraries with type stubs | ||
| from setuptools._distutils.command.install import install | ||
| from setuptools._distutils.dist import Distribution | ||
| else: | ||
| from distutils.command.install import install | ||
| from distutils.dist import Distribution | ||
|
|
||
| cmd: Any = install(Distribution()) | ||
| cmd.finalize_options() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,17 +19,28 @@ | |
| import logging | ||
| import os | ||
| import sys | ||
| from distutils.cmd import Command as DistutilsCommand | ||
| from distutils.command.install import SCHEME_KEYS | ||
| from distutils.command.install import install as distutils_install_command | ||
| from distutils.sysconfig import get_python_lib | ||
| from distutils.command.install import SCHEME_KEYS # type: ignore | ||
| from typing import TYPE_CHECKING, cast | ||
|
|
||
| from pip._internal.models.scheme import Scheme | ||
| from pip._internal.utils.compat import WINDOWS | ||
| from pip._internal.utils.virtualenv import running_under_virtualenv | ||
|
|
||
| from .base import get_major_minor_version | ||
|
|
||
| if TYPE_CHECKING: | ||
| # Vendored libraries with type stubs | ||
|
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. This comment isn't quite right. I think what you've done here is to remove some imports that were only necessary for type checking into this block, as well as using setuptools as an alias for distutils to support some type checking of |
||
| from setuptools._distutils.cmd import Command as DistutilsCommand | ||
| from setuptools._distutils.command.install import ( | ||
| install as distutils_install_command, | ||
| ) | ||
| from setuptools._distutils.dist import Distribution # noqa: F401 | ||
| from setuptools._distutils.sysconfig import get_python_lib | ||
| else: | ||
| from distutils.command.install import install as distutils_install_command | ||
| from distutils.sysconfig import get_python_lib | ||
|
|
||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
|
|
@@ -65,7 +76,7 @@ def distutils_scheme( | |
| obj: DistutilsCommand | None = None | ||
| obj = d.get_command_obj("install", create=True) | ||
|
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 is desirable to avoid cast if we can... is it going to work if we declare: (this requires that |
||
| assert obj is not None | ||
| i: distutils_install_command = obj | ||
| i = cast(distutils_install_command, obj) | ||
| # NOTE: setting user or home has the side-effect of creating the home dir | ||
| # or user base for installations during finalize_options() | ||
| # ideally, we'd prefer a scheme class that has no side-effects. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,7 +125,7 @@ def from_metadata_file_contents( | |
| } | ||
| dist = pkg_resources.DistInfoDistribution( | ||
| location=filename, | ||
| metadata=InMemoryMetadata(metadata_dict, filename), | ||
| metadata=InMemoryMetadata(metadata_dict, filename), # type: ignore[arg-type] | ||
|
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. Personally, I always insist that a Might be a good idea here too, as it would be easy for this to miss a real problem in the future. |
||
| project_name=project_name, | ||
| ) | ||
| return cls(dist) | ||
|
|
@@ -146,7 +146,7 @@ def from_wheel(cls, wheel: Wheel, name: str) -> BaseDistribution: | |
| raise UnsupportedWheel(f"{name} has an invalid wheel, {e}") | ||
| dist = pkg_resources.DistInfoDistribution( | ||
| location=wheel.location, | ||
| metadata=InMemoryMetadata(metadata_dict, wheel.location), | ||
| metadata=InMemoryMetadata(metadata_dict, wheel.location), # type: ignore[arg-type] | ||
| project_name=name, | ||
| ) | ||
| return cls(dist) | ||
|
|
@@ -176,7 +176,7 @@ def installed_by_distutils(self) -> bool: | |
| # provider has a "path" attribute not present anywhere else. Not the | ||
| # best introspection logic, but pip has been doing this for a long time. | ||
| try: | ||
| return bool(self._dist._provider.path) | ||
| return bool(self._dist._provider.path) # type: ignore | ||
| except AttributeError: | ||
| return False | ||
|
|
||
|
|
||
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 dependency groups for docs in this PR? Seems like it could be a separate one...
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.
The motivation is to make everything available as a dependency group for type hinting so that all dependencies are exposed in the pyproject.toml and therefore more "noticeable" by future maintainers / tools.
I have no problem splitting the docs group into a separate PR, it just seemed a small enough change to include here (I initially didn't split them out in my previous PR #13475 because I didn't properly test that it worked, but on making this PR I realized that as long as you've installed pip it does).