SPIKE: Add mypy type checking #173
Closed
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 mostly involves explicitly marking nullable fields (uisng the new-style `Type | None` syntax, rather than `Optional[Type]`). Mypy does have an option to implicitly convert arguments with a null default (i.e. `arg: Type = None`) to nullable. But it's not now recommended.
This is because `"PathItem" | None` is valid typing code but not valid runtime Python. Also because this way is less stringy and more futurey. Adam has a good summary of the issues here: https://adamj.eu/tech/2021/05/15/python-type-hints-future-annotations/
Previously the guarantee that the `AIRLOCK_DEV_USERS_FILE` setting was not null at the point we call `read_text()` on it was implicit: we only ever called the relevant function from a function which checked the setting beforehand. This wasn't enough for mypy (not unreasonably) so now we pass the value through after we check it, allowing mypy to confirm it isn't null.
The `reverse` function happens to exist in `django.shortcuts` because it's used there, but it's only documented as importable from `django.urls` and therefore only appears in the typing stubs for that module.
The type checker doesn't know enough about Django's `SimpleLazyObject` to be able to infer this.
We could supply our own typing stubs here but I can't see how it's worth it for a single function which is only used in this module.
Mypy didn't like reassigning the variable with an expression of a different type.
We need to convince mypy that the return value here is never null, and it's easiest to do that with a separate method.
This is a genuine, albeit, small bug uncovered by mypy – stemming from a misunderstanding of the structure of `form.errors`.
I had been aiming towards passing with the setting `--disallow-untyped-calls` which didn't initially look like too much work. But of course what happens is that the more types you add the more the codebase falls under mypy's purview and the more problems it starts to find. So I'm calling time here and just commiting the changes so far.
| ["french", "French"], | ||
| ["german", "German"], | ||
| ["spanish", "Spanish"], | ||
| ("", "Please select a language"), |
There was a problem hiding this comment.
ugh, we pull this in from job-server, and we don't need it
Can we ignore assets/*? we don't ahve any code we author in there?
We could perhaps remove this and some other python files
There was a problem hiding this comment.
Interesting. Some of those things seem worth being strict about.
I propose ignoring old_api and assets
old_api because its a) temporary and b) is in part imported directly from release-hatch.
assets/ because any python code in there is 100% imported from job-server, which is not type checked.
Merged
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.
This is spike to get a sense of what would be involved in adding type checking to Airlock and how it might benefit us.
This involves installing
mypyand also type stubs and a mypy plugin to get it to play nicely with Django.Worth noting is that the latest release of the Django type stubs supports neither the latest version of mypy nor the latest version Django:
https://github.com/typeddjango/django-stubs?tab=readme-ov-file#version-compatibility
However, in practice it seems to have worked without issue in the sense that I don't think any of the problems I've hit have been the result of django-stubs.
I started by getting
mypypassing on its default settings. This involved a little bit of faff to correct some of our type signatures and a few other bits and pieces. It wasn't too much work, but then neither did it expose any interesting bugs.I then started enabling all the flags involved in
mypy --strictmode. Enabling them one-by-one in rough order of how many issues they seemed to find and making the necessary fixes. This did expose some small genuine bugs, but nothing earth shattering.At first this work progressed quite quickly and I got as far as these three final flags:
However, what I discovered is that
mypyhad been merrily ignoring all the untyped parts of the codebase and the more of it I added types to the more issues it found.Some of the work here is pretty straightforward and mechanical (e.g. adding
-> str:to the end of a lot of function defs) but some of it, in particular the recursive tree walking code where nested structures are getting passed around, is difficult. I think probably the pressure frommypyhere is good in the sense that easier to type code would also be easier to understand. But it's not quite as simple as just slapping a few annotations on and calling it a day.