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: add mypy to test dependencies #1789

Merged
merged 3 commits into from
May 2, 2024
Merged

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Apr 11, 2024

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:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@dstansby
Copy link
Contributor

Since mypy runs as part of pre-commit anyway, how about just getting rid of the GitHub actions mypy stage?

@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 11, 2024

Since mypy runs as part of pre-commit anyway, how about just getting rid of the GitHub actions mypy stage?

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 🤔

@dstansby
Copy link
Contributor

I suspect

continue-on-error: true
might be the culprit 😄

@Saransh-cpp
Copy link
Contributor

pre-commit is running in the CI (as seen in this PR, but maybe only on PRs? pre-commit.ci is faster than GH Actions so it should be preferred if there are no blockers) -

image

@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 11, 2024

pre-commit is running in the CI (as seen in this PR, but maybe only on PRs? pre-commit.ci is faster than GH Actions so it should be preferred if there are no blockers) -
image

I stand corrected! in this case, i'm confused that mypy is failing (silently) in GHA, but passing in pre-commit

@dstansby
Copy link
Contributor

It's because types-redis is installed as part of the pre-commit checks, but not installed on GitHub actions. I think it makes a bit more sense to run mypy on the pre-commit check, to avoid duplicating config between the pre-commit config and GitHub actions setup. Alternatively, in GitHub actions pre-commit run mypy could be run to make sure the environment is the same as the one developers will see when they run pre-commit, but this is kind of what pre-commit.ci is set up to do anyway.

@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 11, 2024

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?

@jhamman
Copy link
Member

jhamman commented Apr 12, 2024

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?

@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 13, 2024

Does the version of python mypy is running on matter? I thought mypy was configured to type-check against a specific python version in pyproject.toml

@dstansby
Copy link
Contributor

Since mypy is a static type checker I don't think it matters which version of Python you run it with, and the results shouldn't depend on that version.

@jhamman jhamman added the V3 Related to compatibility with V3 spec label Apr 22, 2024
@jhamman jhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
@jhamman
Copy link
Member

jhamman commented Apr 24, 2024

This seems worthy of a merge regardless of where we run the CI checks, no?

@jhamman jhamman merged commit c1323c4 into zarr-developers:v3 May 2, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V3 Related to compatibility with V3 spec
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants