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

moved mypy to the make check command #138

Merged
merged 2 commits into from
Sep 28, 2022
Merged

moved mypy to the make check command #138

merged 2 commits into from
Sep 28, 2022

Conversation

fpgmaas
Copy link
Owner

@fpgmaas fpgmaas commented Sep 28, 2022

Proposal to move mypy from tox to make check. This makes sure that:

  • it runs only once instead of four times in the CI/CD pipeline. It will also raise the errors sooner.
  • Makes it easier to run the checks locally (no need to run deptry and mypy separaetely).

Weirdly enough, the files argument does not work for me locally:

(deptry-py3.10) ➜  deptry git:(mypy2) ✗ ls
CONTRIBUTING.rst LICENSE          Makefile         README.md        codecov.yaml     coverage.xml     deptry           docs             poetry.lock      poetry.toml      pyproject.toml   tests            tox.ini
(deptry-py3.10) ➜  deptry git:(mypy2) ✗ mypy --version
mypy 0.981 (compiled: yes)
(deptry-py3.10) ➜  deptry git:(mypy2) ✗ mypy .
tests/test_dependency.py:6: error: Function is missing a return type annotation  [no-untyped-def]
...
Found 107 errors in 17 files (checked 40 source files)

So I reverted it to exclude.

PR Checklist

  • A description of the changes is added to the description of this PR.
  • If there is a related issue, make sure it is linked to this PR.
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

Description of changes

@mkniewallner
Copy link
Collaborator

Oh yeah, I should have explained a bit the reasoning behind that in #135.

Basically, running mypy on multiple Python versions ensures that the typing is correct across versions. This is especially important since we can use different dependencies depending on the version used (for instance, tomli only used in <3.11, or importlib-metadata only used in <3.8).

Of course most of the time, typing will be valid on all versions, so indeed, it's more resources to spend on the CI, but mypy runs pretty fast, so I don't think it matters that much, and it is pretty much the only way to ensure correctness across versions.

For the local development though, I agree that it could be beneficial to have mypy on check. Similarly to the tests that are run both on make test for a single version and on tox for all supported Python versions, what would you think about adding mypy to check as this PR does, but still keep mypy on tox, to also ensure that typing is valid across versions?
That way, developers can still iterate quickly by running make check, and when the code is ready, they run tox to also ensures that both tests and typing are valid for all Python versions.

For the files issue you mention, when files is set, you don't need to pass parameter to mypy anymore. If you run mypy using mypy instead of mypy ., it should only run on the path defined in files, whereas passing an argument will override files.

@fpgmaas
Copy link
Owner Author

fpgmaas commented Sep 28, 2022

@mkniewallner Thanks for your clarification, that makes sense! I agree with your approach, I have added mypy back to tox.

@fpgmaas fpgmaas merged commit ce81ea3 into main Sep 28, 2022
@fpgmaas fpgmaas deleted the mypy2 branch September 28, 2022 15:52
fpgmaas added a commit that referenced this pull request Oct 2, 2022
* moved mypy to the `make check` command
* reset mypy configuration to use the `files` argument, enabled in tox

Co-authored-by: Florian Maas <fpgmaas@gmail.com>
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