Skip to content

Move mypy configs from pre-commit to mypy config #5110

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

Merged
merged 6 commits into from
Apr 14, 2021

Conversation

max-sixty
Copy link
Collaborator

  • Passes pre-commit run --all-files

This should mean that running mypy . doesn't operate differently from running it in pre-commit.

It's also possible I've misunderstood something about mypy configs, please let me know if so!

@max-sixty
Copy link
Collaborator Author

This doesn't seem to work:

conftest.py: error: Duplicate module named 'conftest' (also at './properties/conftest.py')
conftest.py: note: Are you missing an __init__.py? Alternatively, consider using --exclude to avoid checking one of them.
Found 1 error in 1 file (errors prevented further checking)

Not sure exactly why it doesn't ignore the errors given the configs.

I'll close this unless anyone has ideas.

@keewis
Copy link
Collaborator

keewis commented Apr 5, 2021

in order to get this to work you would need to add empty __init__.py to asv_bench and properties. Not sure if we would then need to modify the packaging configuration, though.

@shoyer
Copy link
Member

shoyer commented Apr 6, 2021

Maybe the recommended command should be mypy xarray?

@max-sixty
Copy link
Collaborator Author

max-sixty commented Apr 12, 2021

I think this should now work! TIL toml doesn't have quotations for strings

We still need the entry in .pre-commit-config.yaml, but at least mypy . works now.

@@ -34,6 +34,7 @@ repos:
rev: v0.812
hooks:
- id: mypy
# Copied from setup.cfg
exclude: "properties|asv_bench"
Copy link
Member

@andersy005 andersy005 Apr 12, 2021

Choose a reason for hiding this comment

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

Now that setup.cfg has

[mypy]
exclude = properties|asv_bench|doc

do we still need this exclude option here? It's my understanding that mypy pre-commit hook should be able to retrieve values defined in setup.cfg

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do, unfortunately.

I guess that pre-commit gives it a list of files, and when passed a list of files mypy doesn't honor the exclusions.

@keewis
Copy link
Collaborator

keewis commented Apr 12, 2021

could you change the call in the mypy CI to use python -m mypy . (so we can verify this works)?

@max-sixty
Copy link
Collaborator Author

could you change the call in the mypy CI to use python -m mypy . (so we can verify this works)?

Great done.

I didn't know about that. Why do we have that as well as ci-pre-commit.yaml?

@keewis
Copy link
Collaborator

keewis commented Apr 14, 2021

Why do we have that as well as ci-pre-commit.yaml?

pre-commit does not create a full environment before running mypy, which means that it can't check using external type hints (like the ones from numpy). See #4881 for more explanation.

@max-sixty
Copy link
Collaborator Author

pre-commit does not create a full environment before running mypy, which means that it can't check using external type hints (like the ones from numpy). See #4881 for more explanation.

Thanks! I added a link to that for future travelers

@max-sixty max-sixty merged commit ab4e94e into pydata:master Apr 14, 2021
@max-sixty max-sixty deleted the mypy-exclude branch April 14, 2021 18:43
@max-sixty max-sixty restored the mypy-exclude branch April 19, 2021 06:05
@max-sixty max-sixty deleted the mypy-exclude branch April 19, 2021 06:10
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.

4 participants