-
Notifications
You must be signed in to change notification settings - Fork 184
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
MPP-79: Add mypy for Python type hints #1831
Conversation
This build shows what a "soft failed" mypy looks like: The last build shows a successful mypy build: |
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.
Whoa, is this really ALL the type-checking fixes we have to make across the whole code-base?
No, not at all. This is just the Start Small step of getting |
6626a67
to
e1d2f90
Compare
Re-based on main, and fixed some typing issues from the last few weeks. I tried enabling fx-private-relay/emails/tests/views_tests.py Lines 393 to 394 in dcd5225
I found a related issue at typeddjango/django-stubs#878. The fix is merged, and it looks like it will be released in the next version. I'd like to merge this version, but I'd be OK including more conversion to types. Converting a recent management command, like emails/management/commands/check_health.py, would be easiest. Converting a utility file, like main/emails/utils.py, would be the highest impact. |
✅ Deploy Preview for fx-relay-demo canceled.
|
25cb07b
to
7edb464
Compare
9180197
to
e1e5485
Compare
A few changes from the last comment:
I've learned a ton about mypy and typing, and it really helped when adding code to handle SES complaints and notifications, finding many issues as I worked. However, |
af6dcd1
to
2e53fed
Compare
I made some notes on Converting fx-private-relay to use mypy, which goes into a lot more details about the changes in this PR, and tips on how to use Creating the notes and going through the training suggested a few changes, added to this PR:
I've rebased again, adjusted the configuration, and updated to the latest packages. I also annotated the functions in I'm marking everyone that's gone through training as a possible reviewer - I'll just take one. If you think |
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.
Love to see this!
name: mypy type check | ||
command: mypy --no-incremental --junit-xml job-results/mypy.xml . | ||
has_results: true | ||
allow_fail: true |
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.
Wouldn't we want this one to be blocking?
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.
Yes, eventually. I want to give developers a week or so (or maybe during an upcoming work week) to get the tools installed locally first.
@@ -0,0 +1,44 @@ | |||
# mypy type hints for 3rd party modules |
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.
Nice docs, and located where they're needed!
When generating stubs, add a docstring header to the file that specifies the | ||
`pip` package name and header. This gives us a chance to recognize that the | ||
stub is out-of-date when updating the package. |
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.
Is there a common convention for this? If so, I'm wondering if there's a lint rule that could catch it when this is missing. (Similar to this one for TS type definitions.)
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.
There is not a convention that I'm aware of, but that seems like a good idea. Practically speaking, I might be the only developer creating these for the next few months.
@@ -29,6 +32,9 @@ | |||
|
|||
import dj_database_url | |||
|
|||
if TYPE_CHECKING: | |||
import wsgiref.headers |
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.
Might be my unfamiliarity, but it's not clear to me why this is needed - might warrant a comment.
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.
Oh hmm, I guess it's just to get at a type definition without including it at runtime, similar to TypeScript's type-only imports.
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.
Yes, it is a good analogy. From the docs, which has a usage example and some notes:
A special constant that is assumed to be
True
by 3rd party static type checkers. It isFalse
at runtime.
I thought TYPE_CHECKING
would come up earlier, but this is the first time I needed it in our code.
Add latest mypy, and type stubs for Django, DRF, and Requests. The Django stubs are for Django 3.2, but they are required for the DRF stubs and for pyproject.toml support (used by Black).
Add a generic python_job that checks out the project, runs a command that may fail, and upload results if it generates them. This can be used for formatting and linting checks later. Use the python_job for a mypy test that is allowed to fail for now.
In CircleCI, the code detects User.profile_set, so I get slightly different results.
Add a working
mypy
configuration inpyproject.toml
, with minimal code changes. I've include type stubs for Django, Django REST Framework, and requests. The Django stubs are for 3.2, so it doesn't findUser.profile_set
, but it is necessary for the DRF stubs andpyproject.toml
support, which we'll use for Black.I've added
mypy
checking to CircleCI, using some of the changes I tried in PR #1830. It is overkill for this change, but will make it easier to addblack
andflake8
orpylint
to CI later.I haven't added any pre-commit hooks for mypy yet. I've used pre-commit on other projects, it is fine. This stackoverflow answer suggests you can have the Husky commit hook call
pre-commit
.Testing:
pip install -r requirements.txt
mypy .