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

Fix all mypy issues #3979

Merged
merged 44 commits into from
Mar 5, 2024
Merged

Fix all mypy issues #3979

merged 44 commits into from
Mar 5, 2024

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Jul 9, 2023

Summary of changes

Progress towards #2345
I'm not familiar with tox or this project's CI setup, could I get a hand to ensure that mypy errors are blocking on the CI now that they're all fixed with base configuration?

Changes:

  • Fixed all mypy issues with base configurations
  • Updated mypy.ini to fit the current state of type-checking in this project:
    • Added comments to explain configuration decisions
    • Explicitly target oldest supported Python version
    • Excluded vendored modules, the build folder, dynamic "extern" modules and a file that was causing "duplicate module name" issues with mypy

mypy . --python-version=3.8|3.11 --platform=linux|win32 results:
image

A handful of separate PRs have been created and split from this one following review discussion:

Pull Request Checklist

  • Changes have tests (see my comment/question about enforcing this in the CI)
  • News fragment added in newsfragments/. (should not affect the end user yet)
    (See documentation for details)

pkg_resources/__init__.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@Avasam
Copy link
Contributor Author

Avasam commented Oct 2, 2023

@jaraco (hopefully pinging the right person, you're the one who answered in #2345)

Could I get a review on this? And a hand with the CI checks (as mentioned, I'm not familiar with tox, but I know how to add standalone PR type-checking jobs as GitHub actions)

@jaraco jaraco closed this Oct 22, 2023
@jaraco jaraco reopened this Oct 22, 2023
@jaraco
Copy link
Member

jaraco commented Oct 22, 2023

(closed accidentally)

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

Thanks for the contrib! I've got some serious concerns about the checks for vendored modules, and some other suggestions about the approach, but for the most part it looks good.

* Use `pyproject.toml` instead of `mypy.ini` to merge tooling configurations (it also makes [exclude](https://mypy.readthedocs.io/en/stable/config_file.html#confval-exclude) configuration a bit cleaner as it supports arrays in `.toml` format)
  * If you still prefer having a standalone `mypy.ini`, I'll move it back and adapt the `exclude` config.

I do wish to retain the mypy settings in mypy.ini for two reasons:

  • The skeleton for this project is maintained upstream, which defines the mypy settings in its own file. Moving the settings to a different file makes it difficult to compare and manage the differences with settings inherited from upstream.
  • Piling all project settings in pyproject.toml is a bad idea in general. I've been meaning to write a blog post about it, but the tl;dr is that because there's no ordering or namespacing, the settings lack structure and consistency (compared with mypy which always appears after docs/ and before newsfragments/ and is clearly distinct from towncrier or tox or pytest settings.

pkg_resources/__init__.py Outdated Show resolved Hide resolved
pkg_resources/__init__.py Outdated Show resolved Hide resolved
pkg_resources/__init__.py Outdated Show resolved Hide resolved
pkg_resources/extern/__init__.py Outdated Show resolved Hide resolved
pkg_resources/extern/__init__.py Outdated Show resolved Hide resolved
setuptools/sandbox.py Outdated Show resolved Hide resolved
setuptools/tests/integration/test_pip_install_sdist.py Outdated Show resolved Hide resolved
setuptools/tests/test_bdist_egg.py Show resolved Hide resolved
setuptools/extern/__init__.py Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@Avasam Avasam requested a review from jaraco October 22, 2023 22:43
Avasam and others added 4 commits February 22, 2024 09:52
Co-authored-by: Anderson Bravalheri <andersonbravalheri+github@gmail.com>
Update setuptools/tests/integration/test_pip_install_sdist.py

Co-authored-by: Anderson Bravalheri <andersonbravalheri+github@gmail.com>
@Avasam
Copy link
Contributor Author

Avasam commented Feb 23, 2024

Btw, with over 40 commits and lots of "oops" commits. Is this gonna be squashed merged? Or should I squash my git history?

@abravalheri
Copy link
Contributor

Btw, with over 40 commits and lots of "oops" commits. Is this gonna be squashed merged? Or should I squash my git history?

If you are more comfortable that way, I think you can do is a git rebase -i and organise the commit history in a way that makes sense to you. Then just update the PR with a push-force... Otherwise, if you tell me that it should be squashed as a whole, I am also happy to oblige.

FYI, I recently merged a bugfix contributed by a member of that community that was having problems with building. That affects primarily build_meta, but I see that the file is not changed in this PR, so hopefully it will not cause too much trouble 🤞.

@Avasam
Copy link
Contributor Author

Avasam commented Feb 24, 2024

I'm comfortable and experienced with interactive rebases, but for this PR you can just squash as a whole.

With typeshed I've taken the habit of not force-pushing between reviews, as it is the policy there, since GitHub makes it harder to view changes since last review if the history is force-pushed, since it's based on commit history (unlike DevOps for example, where update history is separate from commits)

@abravalheri abravalheri merged commit 226e1a2 into pypa:main Mar 5, 2024
21 of 23 checks passed
@abravalheri
Copy link
Contributor

Thank you very much @Avasam! That was a heroic piece of work.


I have a doubt:

How are you running mypy? When I try to run mypy --python-version=3.8 --platform=linux ., I still get back 3 errors, which I suppose should be ignored as configured in mypy.ini:

$ mypy --python-version=3.8 --platform=linux .
setuptools/command/build_clib.py:6: error: Cannot find implementation or library stub for module named "distutils._modified"  [import-not-found]
setuptools/command/build_clib.py:6: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
setuptools/config/_validate_pyproject/formats.py:180: error: Cannot find implementation or library stub for module named "trove_classifiers"  [import-not-found]
setuptools/command/build_ext.py:19: error: Cannot find implementation or library stub for module named "Cython.Distutils.build_ext"  [import-not-found]
Found 3 errors in 3 files (checked 146 source files)

I can see that at least setuptools/_distutils and setuptools/config/_validate_pyproject are excluded in mypy.ini.
(I have installed mypy==1.8.0, mypy-extensions==1.0.0, typing_extensions==4.9.0)

Is this something that is supposed to happen?

@Avasam
Copy link
Contributor Author

Avasam commented Mar 5, 2024

@abravalheri Thank you too for your support!

The last two (trove_classifiers and Cython) are just missing installs, although I think Cython isn't installed in CI. For distutils._modified I don't remember why it could differ, maybe because I have an editable install of setuptools 🤷‍♂️

Since I figured out how to enable mypy in CI, I have a very small follow-up PR that does just that an appropriately ignores "missing-imports" that happens on CI with a comment. #4257

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.

3 participants