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

Run mypy in CI with dependencies' type annotations #115

Merged
merged 6 commits into from
Dec 6, 2019
Merged

Run mypy in CI with dependencies' type annotations #115

merged 6 commits into from
Dec 6, 2019

Conversation

hukkin
Copy link

@hukkin hukkin commented Dec 3, 2019

  • Run mypy in an env that has dependencies and their typehints
    • Fix resulting mypy errors
  • Add a few strictness flags for mypy
    • Fix resulting mypy errors

extras = lint
commands =
pre-commit run --all-files
mypy .
Copy link
Owner

Choose a reason for hiding this comment

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

Won't this mean that mypy runs twice--once in pre-commit and once here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it does. In pre-commit it doesn't have the typehints of dependencies' so the checks aren't as comprehensive. If you want, we can remove mypy from pre-commit. I left it because it can still be useful when making a git commit locally.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd be fine removing mypy from pre-commit. the lint job is already the slowest job in CI at the moment, and I'd hate to make it slower. Also, it's one less thing to manually sync between pre-commit-config.yaml and setup.py.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, maybe add a comment above this line explaining why we're running mypy outside of pre-commit

Copy link
Author

Choose a reason for hiding this comment

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

Removed mypy from pre-commit and made the comment in tox.ini

setup.py Show resolved Hide resolved
environs.py Show resolved Hide resolved
@sloria
Copy link
Owner

sloria commented Dec 5, 2019

Thanks. Will give this another look and release after work today or tomorrow.

@sloria sloria merged commit 9da03bf into sloria:master Dec 6, 2019
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.

3 participants