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.

Sign up for GitHub

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

Jump to bottom

Type-check on all Python versions #4352

Merged
merged 1 commit into from
Aug 21, 2024
Merged

Type-check on all Python versions #4352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[mypy]
# CI should test for all versions, local development gets hints for oldest supported
# Some upstream typeshed distutils stubs fixes are necessary before we can start testing on Python 3.12
python_version = 3.8
# But our testing setup doesn't allow passing CLI arguments, so local devs have to set this manually.
# python_version = 3.8
Comment on lines +3 to +4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would love if there was a way to tell the CI to pass the --python-version argument to override this config so that IDEs and maybe default local runs can test against the oldest supported Python version.

Copy link
Contributor

Choose a reason for hiding this comment

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

pytest-mypy mentions the following:

Mypy supports reading configuration settings from a mypy.ini file. Alternatively, the plugin can be configured in a conftest.py to invoke mypy with extra options:

def pytest_configure(config):
    plugin = config.pluginmanager.getplugin('mypy')
    plugin.mypy_argv.append('--check-untyped-defs')

Not sure if that is useful to achieve what you want via conftest.py.

Copy link
Contributor Author

@Avasam Avasam Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it would, something like:

import sys

def pytest_configure(config):
    plugin = config.pluginmanager.getplugin('mypy')
    plugin.mypy_argv.append(f'--python-version={sys.version_info.major}.{sys.version_info.minor}')

Since you approved, mind if I tackle your comments in a follow-up ?

Copy link
Contributor

@abravalheri abravalheri Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since you approved, mind if I tackle your comments in a follow-up ?

Sure!

plugin.mypy_argv.append(f'--python-version={sys.version_info.major}.{sys.version_info.minor}')

Is that going to behave differently than just omitting python_version in mypy.ini?

pytest-mypy would force the contributor to check on the current version anyway, right?

Copy link
Contributor Author

@Avasam Avasam Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Honestly, I never run pytest-mypy locally. But yes that would mean that the mypy-test commands would always check for the executed Python. A more complete snippet of code would probably check if this is being run on the CI (check for GitHub env var) and whether the dev already passed --python-version as to not overwrite it (let's say they run pytest-mypy directly w/o tox)

strict = False
warn_unused_ignores = True
warn_redundant_casts = True
Expand Down Expand Up @@ -30,15 +30,19 @@ disable_error_code = attr-defined
[mypy-pkg_resources.tests.*]
disable_error_code = import-not-found

# - distutils._modified has different errors on Python 3.8 [import-untyped], on Python 3.9+ [import-not-found]
# - distutils doesn't exist on Python 3.12, unfortunately, this means typing
# will be missing for subclasses of distutils on Python 3.12 until either:
# - support for `SETUPTOOLS_USE_DISTUTILS=stdlib` is dropped (#3625)
# for setuptools to import `_distutils` directly
# - or non-stdlib distutils typings are exposed
Comment on lines +35 to +37
Copy link
Contributor Author

@Avasam Avasam Aug 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This includes potentially moving _distutils to a path where the last folder ends in distutils so it can be added to the MYPY_PATH. (although that only helps setuptool's own development, not its downstream Python 3.12+ users)

Or typeshed accepting to publish a non-stdlib types-distutils

Until then, Python 3.12+ users won't be able to have complete typing without types-setuptools (which already provides distutils-stubs https://github.com/python/typeshed/tree/main/stubs/setuptools/distutils) since anything inheriting or re-exported from distutils will be typed as Any.

Anyway, not a huge concern just yet, but something to start thinking about.

# - All jaraco modules are still untyped
# - _validate_project sometimes complains about trove_classifiers (#4296)
# - wheel appears to be untyped
[mypy-distutils._modified,jaraco.*,trove_classifiers,wheel.*]
[mypy-distutils.*,jaraco.*,trove_classifiers,wheel.*]
ignore_missing_imports = True

# Even when excluding a module, import issues can show up due to following import
# https://github.com/python/mypy/issues/11936#issuecomment-1466764006
[mypy-setuptools.config._validate_pyproject.*]
[mypy-setuptools.config._validate_pyproject.*,setuptools._distutils.*]
follow_imports = silent
# silent => ignore errors when following imports
11 changes: 9 additions & 2 deletions setuptools/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
"""Extensions to the 'distutils' for large or complex distributions"""
# mypy: disable_error_code=override
# Command.reinitialize_command has an extra **kw param that distutils doesn't have
# Can't disable on the exact line because distutils doesn't exists on Python 3.12
# and mypy isn't aware of distutils_hack, causing distutils.core.Command to be Any,
# and a [unused-ignore] to be raised on 3.12+
Comment on lines +4 to +6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively:

Suggested change
# Can't disable on the exact line because distutils doesn't exists on Python 3.12
# and mypy isn't aware of distutils_hack, causing distutils.core.Command to be Any,
# and a [unused-ignore] to be raised on 3.12+
# See comment in mypy.ini about distutils on Python 3.12


from __future__ import annotations

Expand Down Expand Up @@ -114,8 +119,10 @@ def setup(**attrs):
setup.__doc__ = distutils.core.setup.__doc__

if TYPE_CHECKING:
from typing_extensions import TypeAlias

# Work around a mypy issue where type[T] can't be used as a base: https://github.com/python/mypy/issues/10962
_Command = distutils.core.Command
_Command: TypeAlias = distutils.core.Command
else:
_Command = monkey.get_unpatched(distutils.core.Command)

Expand Down Expand Up @@ -207,7 +214,7 @@ def ensure_string_list(self, option):
"'%s' must be a list of strings (got %r)" % (option, val)
)

@overload # type:ignore[override] # Extra **kw param
@overload
def reinitialize_command(
self, command: str, reinit_subcommands: bool = False, **kw
) -> _Command: ...
Expand Down
5 changes: 3 additions & 2 deletions setuptools/build_meta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,9 +387,10 @@ def _build_with_temp_dir(

# Build in a temporary directory, then copy to the target.
os.makedirs(result_directory, exist_ok=True)
temp_opts = {"prefix": ".tmp-", "dir": result_directory}

with tempfile.TemporaryDirectory(**temp_opts) as tmp_dist_dir:
with tempfile.TemporaryDirectory(
prefix=".tmp-", dir=result_directory
) as tmp_dist_dir:
sys.argv = [
*sys.argv[:1],
*self._global_args(config_settings),
Expand Down
2 changes: 1 addition & 1 deletion setuptools/command/build_ext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
get_config_var("LDSHARED")
# Not publicly exposed in typeshed distutils stubs, but this is done on purpose
# See https://github.com/pypa/setuptools/pull/4228#issuecomment-1959856400
from distutils.sysconfig import _config_vars as _CONFIG_VARS # type: ignore # noqa
from distutils.sysconfig import _config_vars as _CONFIG_VARS # noqa: E402


def _customize_compiler_for_shlib(compiler):
Expand Down
4 changes: 3 additions & 1 deletion setuptools/command/sdist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from __future__ import annotations

import contextlib
import os
from itertools import chain
Expand Down Expand Up @@ -46,7 +48,7 @@ class sdist(orig.sdist):
]

distribution: Distribution # override distutils.dist.Distribution with setuptools.dist.Distribution
negative_opt = {}
negative_opt: dict[str, str] = {}

README_EXTENSIONS = ['', '.rst', '.txt', '.md']
READMES = tuple('README{0}'.format(ext) for ext in README_EXTENSIONS)
Expand Down
5 changes: 4 additions & 1 deletion setuptools/config/setupcfg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@
Generic,
Iterable,
Iterator,
List,
Tuple,
TypeVar,
Union,
cast,
)

from packaging.markers import default_environment as marker_env
Expand Down Expand Up @@ -108,7 +110,8 @@ def _apply(
filenames = [*other_files, filepath]

try:
_Distribution.parse_config_files(dist, filenames=filenames) # type: ignore[arg-type] # TODO: fix in distutils stubs
# TODO: Temporary cast until mypy 1.12 is released with upstream fixes from typeshed
_Distribution.parse_config_files(dist, filenames=cast(List[str], filenames))
Comment on lines +113 to +114
Copy link
Contributor Author

Choose a reason for hiding this comment

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

python/typeshed@ac8720b#diff-d648cd8f6aa0211e1f3e96439ba4a67ec8b7474fbf0aba2459b894cb5022a0bcR16

handlers = parse_configuration(
dist, dist.command_options, ignore_option_errors=ignore_option_errors
)
Expand Down
4 changes: 3 additions & 1 deletion setuptools/dist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,10 @@ def check_packages(dist, attr, value):


if TYPE_CHECKING:
from typing_extensions import TypeAlias

# Work around a mypy issue where type[T] can't be used as a base: https://github.com/python/mypy/issues/10962
_Distribution = distutils.core.Distribution
_Distribution: TypeAlias = distutils.core.Distribution
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one we can do:

Suggested change
_Distribution: TypeAlias = distutils.core.Distribution
from distutils.core import Distribution as _Distribution

and remove the import for TypeAlias.

(errors.py should be fine using the TypeAlias though - I am indifferent on the approach for that file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since you already approved and these are changed in #4504 , I'll keep it as-is for this PR

else:
_Distribution = get_unpatched(distutils.core.Distribution)

Expand Down
41 changes: 24 additions & 17 deletions setuptools/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,36 @@
Provides exceptions used by setuptools modules.
"""

from __future__ import annotations

from typing import TYPE_CHECKING

from distutils import errors as _distutils_errors

if TYPE_CHECKING:
from typing_extensions import TypeAlias

# Re-export errors from distutils to facilitate the migration to PEP632

ByteCompileError = _distutils_errors.DistutilsByteCompileError
CCompilerError = _distutils_errors.CCompilerError
ClassError = _distutils_errors.DistutilsClassError
CompileError = _distutils_errors.CompileError
ExecError = _distutils_errors.DistutilsExecError
FileError = _distutils_errors.DistutilsFileError
InternalError = _distutils_errors.DistutilsInternalError
LibError = _distutils_errors.LibError
LinkError = _distutils_errors.LinkError
ModuleError = _distutils_errors.DistutilsModuleError
OptionError = _distutils_errors.DistutilsOptionError
PlatformError = _distutils_errors.DistutilsPlatformError
PreprocessError = _distutils_errors.PreprocessError
SetupError = _distutils_errors.DistutilsSetupError
TemplateError = _distutils_errors.DistutilsTemplateError
UnknownFileError = _distutils_errors.UnknownFileError
ByteCompileError: TypeAlias = _distutils_errors.DistutilsByteCompileError
CCompilerError: TypeAlias = _distutils_errors.CCompilerError
ClassError: TypeAlias = _distutils_errors.DistutilsClassError
CompileError: TypeAlias = _distutils_errors.CompileError
ExecError: TypeAlias = _distutils_errors.DistutilsExecError
FileError: TypeAlias = _distutils_errors.DistutilsFileError
InternalError: TypeAlias = _distutils_errors.DistutilsInternalError
LibError: TypeAlias = _distutils_errors.LibError
LinkError: TypeAlias = _distutils_errors.LinkError
ModuleError: TypeAlias = _distutils_errors.DistutilsModuleError
OptionError: TypeAlias = _distutils_errors.DistutilsOptionError
PlatformError: TypeAlias = _distutils_errors.DistutilsPlatformError
PreprocessError: TypeAlias = _distutils_errors.PreprocessError
SetupError: TypeAlias = _distutils_errors.DistutilsSetupError
TemplateError: TypeAlias = _distutils_errors.DistutilsTemplateError
UnknownFileError: TypeAlias = _distutils_errors.UnknownFileError

# The root error class in the hierarchy
BaseError = _distutils_errors.DistutilsError
BaseError: TypeAlias = _distutils_errors.DistutilsError


class InvalidConfigError(OptionError):
Expand Down
4 changes: 3 additions & 1 deletion setuptools/extension.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@ def _have_cython():
# for compatibility
have_pyrex = _have_cython
if TYPE_CHECKING:
from typing_extensions import TypeAlias

# Work around a mypy issue where type[T] can't be used as a base: https://github.com/python/mypy/issues/10962
_Extension = distutils.core.Extension
_Extension: TypeAlias = distutils.core.Extension
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_Extension: TypeAlias = distutils.core.Extension
from distutils.core import Extension as _Extension

And then we can drop the TypeAlias.

else:
_Extension = get_unpatched(distutils.core.Extension)

Expand Down
Loading