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

Enable mypy on CI #4257

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Enable mypy on CI #4257

merged 1 commit into from
Mar 6, 2024

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Mar 5, 2024

Summary of changes

Follow-up to #3979. Extracted from #4192. Enables mypy type-checking in the CI.

Example failure: https://github.com/pypa/setuptools/actions/runs/8158516102/job/22300634747?pr=4257#step:9:69

Pull Request Checklist

  • Changes have tests (this is enabling the tests! 😄 )
  • News fragment added in newsfragments/. (no user-facing change)
    (See documentation for details)

@Avasam Avasam mentioned this pull request Mar 5, 2024
2 tasks
Avasam added a commit to Avasam/skeleton that referenced this pull request Mar 5, 2024
pypa/setuptools#4257 shows that mypy now works with PyPy
setup.cfg Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@Avasam Avasam force-pushed the enable-mypy-on-ci branch from 81dfd0a to 2ee6ac9 Compare March 5, 2024 15:58
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@Avasam Avasam mentioned this pull request Mar 6, 2024
2 tasks
setup.cfg Outdated
@@ -66,13 +66,16 @@ testing =
filelock>=3.4.0
ini2toml[lite]>=0.9
tomli-w>=1.0.0
tomli
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that this is to cover

?

If we are going for that approach, would it make more sense to remove the testing-integration extra and just use testing for everything?

Copy link
Contributor Author

@Avasam Avasam Mar 6, 2024

Choose a reason for hiding this comment

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

I suppose that this is to cover ... ?

Yes, same reason as importlib_metadata, with tomli not directly required by any dependency on 3.11

I don't see an issue with having less dependencies in testing-integration. And if you wanna merge them together that's up to you.

If you want to reduce duplication, then I think my latest commit should help with that (assuming it works)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @Avasam, I was just asking your option about the matter, sorry if it prompted a change.

I think what we do in the PR is to just leave as you wrote it before: not touching testing-integration and just adding the extras in testing.

In a separated PR, I think we just remove the testing-integration and use testing everywhere...

Although it might add a bit of overhead for the integration tests, it will simplify and streamline the setup... This way we reduce the divergence with skeleton. The additional overhead in the integration tests should not be too much, and the integration tests just run before the releases anyway.

Copy link
Contributor Author

@Avasam Avasam Mar 6, 2024

Choose a reason for hiding this comment

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

sorry if it prompted a change.

I actually had the deduplication commit ready already, so it's more of a coincidence ^^

I think what we do in the PR is to just leave as you wrote it before

np, I'll revert that change and leave modifications to setup.cfg that are not absolutely necessary to another PR.

The additional overhead in the integration tests should not be too much

Especially if the pypi downloads are cached anyway (are they?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Especially if the pypi downloads are cached anyway (are they?)

I don't remember now if the CI is keeping the pip cache 😅. But it should not matter that much.

I just hope someone is not installing setuptools[testing-integration]. But that is unlikely in my opinion.

setup.cfg Outdated Show resolved Hide resolved
@Avasam Avasam force-pushed the enable-mypy-on-ci branch from 08dc3be to a33c387 Compare March 6, 2024 16:28
@abravalheri abravalheri merged commit a89efc5 into pypa:main Mar 6, 2024
23 checks passed
@abravalheri
Copy link
Contributor

Thank you very much @Avasam!

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