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

MPP-79: Add mypy for Python type hints #1831

Merged
merged 21 commits into from
Jun 29, 2022
Merged

MPP-79: Add mypy for Python type hints #1831

merged 21 commits into from
Jun 29, 2022

Conversation

jwhitlock
Copy link
Member

Add a working mypy configuration in pyproject.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 find User.profile_set, but it is necessary for the DRF stubs and pyproject.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 add black and flake8 or pylint 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 .
  • Look at CircleCI workflow results

@jwhitlock jwhitlock changed the title Add mypy for Python type hints MPP-79: Add mypy for Python type hints May 2, 2022
Copy link
Member

@groovecoder groovecoder left a 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?

.circleci/config.yml Outdated Show resolved Hide resolved
@jwhitlock
Copy link
Member Author

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 mypy to run clean against an existing code base. As we add typing hints to the existing code, or override specific files to run in strict mode, we'll have to fix issues and make changes.

@jwhitlock jwhitlock force-pushed the add-mypy branch 2 times, most recently from 6626a67 to e1d2f90 Compare May 9, 2022 14:19
@jwhitlock
Copy link
Member Author

Re-based on main, and fixed some typing issues from the last few weeks.

I tried enabling warn_unused_ignores. This ignore is needed in my development environment, but is not needed in CircleCI:

self.user = baker.make(User, email="[email protected]", make_m2m=True)
self.profile: Optional[Profile] = self.user.profile_set.first() # type: ignore[attr-defined]

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.

@jwhitlock jwhitlock requested review from groovecoder and say-yawn and removed request for groovecoder May 9, 2022 14:35
@netlify
Copy link

netlify bot commented May 20, 2022

Deploy Preview for fx-relay-demo canceled.

Name Link
🔨 Latest commit 6a2ecf0
🔍 Latest deploy log https://app.netlify.com/sites/fx-relay-demo/deploys/62bb2b3b5bfaaf0008a95ec0

@jwhitlock
Copy link
Member Author

A few changes from the last comment:

  • I added a typing stub for decouple, and added more types to privaterelay.settings
  • Updated to mypy 0.950, django-stubs 1.11.0, and djangorestframework-stubs 1.6.0. Weirdness between my dev and CircleCI has gone away.

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, mypy is not like a lot of linters, where the changes are trivial and easy. There is a ton of new concepts, new tools, etc. I think the next step is a live introduction to mypy, showing how you'd get from our current state to no errors. I'll try to schedule that this month, for at least the backend devs.

@jwhitlock jwhitlock force-pushed the add-mypy branch 4 times, most recently from af6dcd1 to 2e53fed Compare June 28, 2022 16:14
@jwhitlock
Copy link
Member Author

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 mypy. I used the notes for two-hour trainings with @say-yawn, @groovecoder, and @Vinnl.

Creating the notes and going through the training suggested a few changes, added to this PR:

  • mypy_stubs/decouple.pyi is better with unused code deleted than commented out. config is easier to understand if typed as a function rather than an implementation of Config.
  • It looks better to use list[str] than tuple[str, ...], and other adjustments that make mypy happy and make for better code.
  • django_stubs_ext.monkeypatch() exists, is nice
  • The types-pyOpenSSL package works nicely
  • The env folder (created by virtualenv during project setup) should be ignored if it exists

I've rebased again, adjusted the configuration, and updated to the latest packages. I also annotated the functions in privaterelay/settings.py, so that everything in there is annotated. I can't mark it for strict=True yet, because it required annotations in models first.

I'm marking everyone that's gone through training as a possible reviewer - I'll just take one. If you think mypy is a bad addition, now is a good time to say that as well!

@jwhitlock jwhitlock requested review from groovecoder and Vinnl June 28, 2022 17:19
Copy link
Collaborator

@Vinnl Vinnl left a 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
Copy link
Collaborator

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?

Copy link
Member Author

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
Copy link
Collaborator

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!

Comment on lines +36 to +38
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.
Copy link
Collaborator

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.)

Copy link
Member Author

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
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Member Author

@jwhitlock jwhitlock Jun 29, 2022

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 is False at runtime.

I thought TYPE_CHECKING would come up earlier, but this is the first time I needed it in our code.

jwhitlock added 21 commits June 29, 2022 12:58
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.
@jwhitlock jwhitlock merged commit 4bcb27f into main Jun 29, 2022
@jwhitlock jwhitlock deleted the add-mypy branch June 29, 2022 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M a few days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants