Skip to content

fix: add mypy to test dependencies - #1789

Merged
jhamman merged 3 commits into
zarr-developers:v3from
d-v-b:add_mypy_dep
May 2, 2024
Merged

fix: add mypy to test dependencies#1789
jhamman merged 3 commits into
zarr-developers:v3from
d-v-b:add_mypy_dep

Conversation

@d-v-b

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@d-v-b

d-v-b commented Apr 11, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

I suspect

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

@Saransh-cpp

Copy link
Copy Markdown
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

d-v-b commented Apr 11, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

d-v-b commented Apr 11, 2024

Copy link
Copy Markdown
Contributor Author

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

jhamman commented Apr 12, 2024

Copy link
Copy Markdown
Member

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

d-v-b commented Apr 13, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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 label Apr 22, 2024
@jhamman jhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
@jhamman

jhamman commented Apr 24, 2024

Copy link
Copy Markdown
Member

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants