-
-
Notifications
You must be signed in to change notification settings - Fork 278
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: add mypy to test dependencies #1789
Conversation
Since |
We don't run pre-commit as part of CI (and I don't think we necessarily want to) so I do think it makes sense to have mypy as a stage, but I'm open to other opinions here. I am noticing that even with mypy running and reporting errors, CI is still green 🤔 |
I suspect zarr-python/.github/workflows/test-v3.yml Line 41 in 3a9d968
|
It's because |
I'm open to either running mypy in pre-commit or as a stage in CI, but it doesn't seem like we need to run it in both places. It looks like @dstansby and @Saransh-cpp recommend the pre-commit approach; does anyone disagree with this, or otherwise object to removing the mypy stage from CI? |
I've found running mypy in pre-commit to be a bit hard to manage -- mostly because we only execute mypy for one Python version. Perhaps there are ways to make sure we have mypy coverage across our matrix of supported Python versions? |
Does the version of python mypy is running on matter? I thought mypy was configured to type-check against a specific python version in |
Since |
This seems worthy of a merge regardless of where we run the CI checks, no? |
mypy was not in our dev dependencies. this PR adds it. expect CI to fail as mypy actually does something, instead of silently erroring out.
closes #1788
TODO: