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

Type-check on all Python versions #4352

Merged
merged 1 commit into from
Aug 21, 2024
Merged

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented May 9, 2024

Summary of changes

Actually type-check targeting all Python versions in CI, including 3.12 & 3.13
This is important validation for #2345

I'd rather if it was possible for the CI to tell mypy which python version to target to override python_version config.

Pull Request Checklist

  • Changes have tests (these are the test changes)
  • News fragment added in newsfragments/. (not public-facing)
    (See documentation for details)

@Avasam Avasam marked this pull request as draft May 10, 2024 01:17
@Avasam

This comment was marked as resolved.

@Avasam

This comment was marked as resolved.

@Avasam

This comment was marked as resolved.

@Avasam Avasam mentioned this pull request Jul 20, 2024
2 tasks
setuptools/command/install.py Outdated Show resolved Hide resolved
Comment on lines +4 to +6
# 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+
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

@Avasam Avasam force-pushed the type-check-3.12 branch 3 times, most recently from 50407ee to 2cec4fc Compare August 10, 2024 04:02
@Avasam Avasam marked this pull request as ready for review August 10, 2024 04:02
Comment on lines +3 to +4
# But our testing setup doesn't allow passing CLI arguments, so local devs have to set this manually.
# python_version = 3.8
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

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

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

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)

Comment on lines +35 to +37
# - support for `SETUPTOOLS_USE_DISTUTILS=stdlib` is dropped (#3625)
# for setuptools to import `_distutils` directly
# - or non-stdlib distutils typings are exposed
Copy link
Contributor Author

@Avasam Avasam Aug 10, 2024

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.

Comment on lines +113 to +114
# TODO: Temporary cast until mypy 1.12 is released with upstream fixes from typeshed
_Distribution.parse_config_files(dist, filenames=cast(List[str], filenames))
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 changed the title Type-check on Python 3.12 Type-check on Python 3.12+ Aug 10, 2024
@Avasam Avasam changed the title Type-check on Python 3.12+ Type-check on all Python versions Aug 21, 2024
Comment on lines +3 to +4
# But our testing setup doesn't allow passing CLI arguments, so local devs have to set this manually.
# python_version = 3.8
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.

# 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

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

@abravalheri
Copy link
Contributor

Thank you very much, @Avasam.

@abravalheri abravalheri merged commit 6079542 into pypa:main Aug 21, 2024
21 checks passed
@Avasam Avasam deleted the type-check-3.12 branch August 21, 2024 15:30
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.

2 participants