-
Notifications
You must be signed in to change notification settings - Fork 7
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 type checking to our CI process #180
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This brings me joy. 🚀 |
jonathangreen
force-pushed
the
feature/mypy
branch
from
February 9, 2022 00:33
c6da170
to
14af7f3
Compare
- Temporarily solves pallets/markupsafe#284 - Remove `markupsafe` entry once `jinja2` has been upgraded.
tdilauro
approved these changes
Mar 2, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! ✔️
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add Mypy type checking to our CI process.
Motivation and Context
As we are adding type annotation in our code base, it is nice to use them not just for understanding, but to actually type check our code. This shouldn't add any runtime overhead, only when we are actually running the type checker. This also prevents us from adding misleading annotations by mistake, since the annotations we add will actually be checked.
This PR takes the approach recommended in the mypy docs for existing code bases.
The files we are running mypy on are defined in
pyproject.toml
. This PR runs the type checker on:core
tests/core
It added annotations where necessary to get the checker to run, and corrected annotations where they were incorrect.
In some of the code, I converted the older python 2 style doc comments with types to be actual type annotations, and removed the old comments, so they wouldn't get out of sync with the annotations. I used
doc484
andcom2ann
to do the conversion, then did some hand correction of the annotations.In the
core/model/hassessioncache.py
I modified the type annotations to better reflect whats going on in the code. Originally the type annotations were using aprotocol
, but this code is actually better suited to be typed as a generic.Documentation
I added some documentation on mypy to the notion wiki:
https://www.notion.so/lyrasis/Mypy-d97f188926294d4c89ceff91ce9556bf
It covers some of the gotchas that I encountered and workarounds used when preparing this PR.
How Has This Been Tested?
Testing has been confined to running the type checker and running the unit tests.