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

Enable static type checks using mypy in CI #601

Merged
merged 6 commits into from
Apr 10, 2024

Conversation

mthuurne
Copy link
Contributor

@mthuurne mthuurne commented Mar 25, 2024

This is a first step in adding type annotations. See #558 for discussion.

Some things to pay attention to during review:

I'm using from __future__ import annotations to force postponed evaluation of annotations. This has several advantages:

  • it allows modern syntax like dict[str, str] or int | None to be used in Python versions where the interpreter doesn't support that syntax yet
  • it avoids most problems when using a type argument to for example QuerySet in Django 3.2, where QuerySet does not yet inherit from typing.Generic
  • it allows using types in annotations that are defined later in the source file
  • it allows using types in annotations that we can't import at runtime because it would create a circular import

I added typing_extensions as a runtime dependency. This makes it possible to use both runtime and static features from newer versions of Python in older versions as well. In particular, typing.Self simplifies a lot of annotations, but was only added to the standard library in Python 3.11.

It is possible to only use static features of typing_extensions and put the import within a if TYPE_CHECKING: guard. It makes maintenance a bit more cumbersome, but it would eliminate the runtime dependency. As a lot of other libraries are using typing_extensions already, I'm not sure if it avoiding the dependency in django-model-utils would actually make a difference in any real-world application. But if you want me to make that change, let me know.

I've used Python 3.8 in the tox configuration to run mypy, because that is the one that also runs flake8 and isort. I'm not sure that's a good reason though. Another option would be to use Python 3.12, as that is the fastest. Note that mypy's results do not depend on the Python version it runs under: it always has access to all typing features it supports.

The type checking of the unit tests requires all imported libraries to exist, which is why I added time_machine as a dependency of the mypy env in tox. If we want to avoid that duplication, maybe it would make sense to create a requirements-testlib.txt or add a testlib extra to setup.py.

It would also be possible to move time_machine into requirements-test.txt, but installing the test tools and runtime dependencies like psycopg2-binary isn't necessary for mypy to work and I wanted to avoid slowing down CI.

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.94%. Comparing base (4c9d6ee) to head (c75e54a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #601   +/-   ##
=======================================
  Coverage   98.94%   98.94%           
=======================================
  Files           6        6           
  Lines         757      757           
=======================================
  Hits          749      749           
  Misses          8        8           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mthuurne
Copy link
Contributor Author

In the summary, I wrote:

As a lot of other libraries are using typing_extensions already, I'm not sure if it avoiding the dependency in django-model-utils would actually make a difference in any real-world application.

In our own Django application, it turns out only scipy depends on typing_extensions, and it has it as an optional dev dependency rather than a runtime dependency. So perhaps I overestimated how many libraries are using it.

@foarsitter
Copy link
Contributor

Thans for your throughout description of this PR.

Is the typing_extensions thing something we can solve in another way?

@mthuurne
Copy link
Contributor Author

mthuurne commented Apr 5, 2024

Is the typing_extensions thing something we can solve in another way?

I used it in several places in earlier versions of the annotations, but in the latest version there are no uses left. I'll drop it from this PR and we can see whether or not to add it later if/when there is a concrete use case to discuss.

This does not require annotations yet, but it will check all code
outside of functions.
Suppress rather than annotate, because people type checking against
`django-model-utils` will always have it installed and therefore
should not have to deal with `__version__` being `None`.
Django can handle both strings and integers, but typeshed expects the
default value to match the mapping's value type.
Avoid using `Self` as a type argument: for some reason this fails
when mypy has an empty cache, but passes when the cache has been
filled. Maybe it's a weird interaction between the mypy core and
the django-stubs plugin?
@foarsitter foarsitter merged commit 0fcfc11 into jazzband:master Apr 10, 2024
9 checks passed
@mthuurne mthuurne deleted the mypy-in-ci branch April 10, 2024 12:05
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.

2 participants