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

Add mypy to CI #177

Merged
merged 11 commits into from
Oct 30, 2023
Merged

Add mypy to CI #177

merged 11 commits into from
Oct 30, 2023

Conversation

vitalk
Copy link
Collaborator

@vitalk vitalk commented Oct 29, 2023

PR provides some type hints (as per #176) and adjusts GitHub CI to respect them.

  • Add type hints to code base
  • New environment to check type hints was added to tox
  • tox configuration was updated to keep python envs consistent with GitHub CI (py37 was dropped, py311 and py312 were added)

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in docs/CHANGELOG.rst, summarizing the change and linking to the issue.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and make sure no tests failed.

@vitalk vitalk merged commit edd1866 into master Oct 30, 2023
12 checks passed
@vitalk vitalk deleted the 176-add-mypy-to-ci branch October 30, 2023 07:39
@@ -1,6 +1,6 @@
[tox]
envlist =
py{37,38,39,linting}
py{38,39,310,311,312,linting},mypy
Copy link
Member

@nicoddemus nicoddemus Oct 30, 2023

Choose a reason for hiding this comment

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

Note that this mypy environment is not running in CI.

I suggest actually that we run mypy in CI by configuring it in .pre-commit-config.yaml instead. Here's how pytest does it:

https://github.com/pytest-dev/pytest/blob/8fb7e8b31efaa55e760c142e26eb82b42081ca28/.pre-commit-config.yaml#L60-L72

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch @nicoddemus

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