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 mypy pre-commit settings #148

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Conversation

potiuk
Copy link
Contributor

@potiuk potiuk commented Nov 17, 2024

When proposing #147 I noticed that mypy failed with missing types-request - and while the mypy configuration had the --install-types, it did not work automatically for requests.

The error was:

cherry_picker/cherry_picker.py:15: error: Library stubs not installed for
"requests"  [import-untyped]
    import requests
    ^
cherry_picker/cherry_picker.py:15: note: Hint: "python3 -m pip install types-requests"
cherry_picker/cherry_picker.py:15: note: (or run "mypy --install-types" to install all missing stub packages)
cherry_picker/cherry_picker.py:15: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports

I looked a bit closer and noticed the advice from
https://github.com/pre-commit/mirrors-mypy

Note that using the --install-types is problematic. Mutating the
pre-commit environment at runtime breaks cache and will break parallel builds.

This PR fixes the problem by removing --install-types and installing types-requests explicitly as additional-dependencies.

starting

When proposing python#147 I noticed that mypy failed with missing
`types-request` - and while the mypy configuration had
the `--install-types`, it did not work automatically for requests.

The error was:

```
cherry_picker/cherry_picker.py:15: error: Library stubs not installed for
"requests"  [import-untyped]
    import requests
    ^
cherry_picker/cherry_picker.py:15: note: Hint: "python3 -m pip install types-requests"
cherry_picker/cherry_picker.py:15: note: (or run "mypy --install-types" to install all missing stub packages)
cherry_picker/cherry_picker.py:15: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
```

I looked a bit closer and noticed the advice from
https://github.com/pre-commit/mirrors-mypy

> Note that using the --install-types is problematic. Mutating the
pre-commit environment at runtime breaks cache and will break parallel
builds.

This PR fixes the problem by removing `--install-types` and installing
types-requests explicitly as additional-dependencies.

starting
@hugovk
Copy link
Member

hugovk commented Nov 17, 2024

See also #144.

@potiuk
Copy link
Contributor Author

potiuk commented Nov 17, 2024

Ah. Yeahl. I see. Feel free to close it, just note this thing as well:

Note that using the --install-types is problematic. Mutating the
pre-commit environment at runtime breaks cache and will break parallel builds.

It's also the reason (it already hit us back in the past) why we don not use --install-types in Airflow but pre-install all needed types stubs:

https://github.com/apache/airflow/blob/main/hatch_build.py#L215

Mypy 0.900 and above ships only with stubs from stdlib so if we need other stubs, we need to install them
manually as types-*. See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
for details. We want to install them explicitly because we want to eventually move to
mypyd which does not support installing the types dynamically with --install-types

So you might consider removing both --installl-types and --non-interactive (the latter is only accepted when --install-types is used).

@hugovk hugovk merged commit 452a9b2 into python:main Nov 21, 2024
26 checks passed
@hugovk
Copy link
Member

hugovk commented Nov 21, 2024

Thank you!

@hugovk hugovk mentioned this pull request Nov 21, 2024
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