Open mthuurne opened 1 year ago
The work-in-progress can be found in this branch.
Given the relatively low line count, I thought I would be able to annotate all of django-model-utils
quickly, but it is quite complex code that does things that are not easy to accommodate in the type system.
The following commits might be worth reviewing already, as they don't contain type annotations themselves, but are code cleanups necessary to make mypy accept the code:
ConnectionDoesNotExist
is imported fromInheritanceQuerySetMixin._clone()
I don't know yet when I'll have time to finish the annotations, but I'll submit the fixes as separate PRs, both to make sure they can be included sooner and to simplify the review of the annotations PR later.
Hi! I was wondering whether you have made any progress on this? I would like to use types too. Can I help?
I haven't made progress since last year. I'm still waiting for my cleanups PRs #566 and #567 to be reviewed.
@mthuurne both PR's you are mentioning are merged. Are you willing to take the further lead on typing?
I'll pick it up again.
I split off one cleanup that can already be applied: #591.
More cleanups: #594, #595, #596.
Even more cleanups: #597, #598.
I also updated the branch on top of the latest master
.
Type annotations isn't a job that has to be done in one haul but you are getting pretty far. By any change, is there a way to make smaller chunks out of the branch?
Yes, there should be ways to carve it up into multiple PRs. I prefer to first annotate everything in a branch though before submitting the annotations themselves, as that can uncover mistakes made earlier. In particular, having the unit tests annotated functions as a consistency check for the annotations in the code under test.
Something that might be possible to split off already is adding mypy to CI. I'll have a look at that later today.
Can you open a draft PR from the annotations branch so we can stay in touch with your progres? If we first merge all the low hanging fruit we may split up the remaining issues into sub branches when needed. Played around with the generic tests of the Tracker
and there may be some challenges that aren't blocking features to me.
I could do that, but I'm still force-pushing after squashing corrections, so that would make the PR rather messy. If you don't mind that, I'll create a draft PR.
I made the PR to add mypy to CI: #601.
I put several discussion points in the PR comment, mostly about how to handle various dependencies.
I could do that, but I'm still force-pushing after squashing corrections, so that would make the PR rather messy. If you don't mind that, I'll create a draft PR.
I don't mind, notifications for drafts is opt-in isn't it?
I don't mind, notifications for drafts is opt-in isn't it?
Yes, but I couldn't create a draft PR from the start: I didn't see any checkbox on the submission form and starting the title with "Draft:" like in GitLab didn't work. So if you want to skip notifications, you'll have to unsubscribe from the PR, I'm afraid.
In any case, what I was worried about is that comments on changes would become orphans as new versions are force-pushed.
It is behind the dropdown:
Thanks for your draft, a lot to learn for me about typing!
Here is a new series of cleanups that can be merged separately: #604, #605, #606, #607.
With 4.5.0 released we can merge our more impactful changes 🎉
Another small cleanup that can be merged: #610.
I'm still working on completing the annotations, but it's progressing more slowly as the low-hanging fruit has already been picked.
PR #601 could already be reviewed and merged: it would at least ensure that new developments do not regress the type checking.
I like your progress @mthuurne
If you are running out of time or want to subdivide it into smaller issues we can merge it as is and take that as starting point. Just let me know.
One thing to decide is how to deal with mixins in the test suite. Unfortunately, there is no good way to handle mixins in Python's typing system yet.
The problem with mixins is that they have expectations on what is available on self
, coming from a base class that they will eventually be mixed in with. However, there is no way to tell a type checker like mypy what the expected base class will be.
If there is only limited functionality used from the base class, I typically duplicate the annotation on the mixin, like the _db
attribute in this example:
class SoftDeletableManagerMixin(Generic[ModelT]):
"""
Manager that limits the queryset by default to show only not removed
instances of model.
"""
_queryset_class = SoftDeletableQuerySet
_db: str | None
...
If a lot of functionality is used from the base class, I typically give the mixin an explicit base class during type checking, but not at runtime:
if TYPE_CHECKING:
MixinBase = TestCase
else:
MixinBase = object
class FieldTrackerMixin(MixinBase):
...
This can become quite tricky though, as you have to be careful to not break the method resolution order during type checking. Also there is the django-stubs mypy plugin, which executes code while TYPE_CHECKING
is True
and will therefore receive an impure mixin.
A full example of this is 869c6a6a6e85c8929b862133c538f61166690e7e, where I had to do some code surgery to get the type annotation overrides and method resolution order correct.
In the particular case of unit tests, the main reason why mixins require special attention is because unittest.TestCase
or django.test.TestCase
is used to get access to methods like assertEqual()
. It is possible to sidestep this issue by using pytest's augmented assert
statements instead and having test functions rather than test methods.
I used the pytest assert
style in parts of 80dda9fada0be16c8fbf054c0d3b96a6d5a13757.
What would be the preferred way forward:
assertX()
methods in each mixin?Personally I prefer the pytest style even when not dealing with mixins, but I can't speak for all developers who will work on this code in the future.
Another small cleanup: #611. This one is backwards incompatible, but that means now is a good time to do that, right?
Yes, 5.0 will be full of breaking changes :)
Note that #541 is still an open issue. For now, I'll annotate the current behavior and drop the "fix" I had for this issue, since it probably wasn't the right fix.
More cleanups: #612, #613, #614
The draft PR #603 passed CI checks for the first time.
There is still work to do though:
py.typed
marker and start checking applications against the annotated libraryAny
- check whether the annotations can be done in a cleaner way
- split up the changes in a way that allows reviewing and merging in chunks
Those last two bullets aren't required (anymore) from my perspective.
Do you guys believe an extra pair of hands would help in closing this issue and releasing 5.x ? @foarsitter @mthuurne
The bulk of the work has been done: everything is covered in annotations. Now it just needs some verification and cleanups.
Ways to help would be:
django-model-utils
against itNote to anyone testing the new annotations: do not use pip install -e
, as the django-stubs
mypy plugin does not see modules installed in that way. This results in a "Could not resolve manager type" error when using InheritanceManager
.
Using just pip install <path>
or pip install <url>
works fine.
If we are in the testing phase I would like to release a beta, what do you people think?
I agree @foarsitter , let's make a release candidate 🚀 .
Some things to discuss before doing a production release:
typing.Self
?Like Django itself, django-model-utils
uses a lot of mix-ins. Currently, I have annotated mix-in methods that return an instance of the concrete class using the mix-in to return the matching concrete class from django-model-utils
. For example:
class InheritanceQuerySetMixin(Generic[ModelT]):
def select_subclasses(self, *subclasses: str | type[models.Model]) -> InheritanceQuerySet[ModelT]:
...
class InheritanceQuerySet(InheritanceQuerySetMixin[ModelT], QuerySet[ModelT]):
...
However, it is also possible for user code to combine InheritanceQuerySetMixin
with their own class and that is in fact the entire reason for having mix-ins in the first place. In that case, the returned instance will be of the user class and the annotation doesn't reflect that.
This can be solved using typing.Self
and that is also how django-stubs
handles situations like this. However, typing.Self
was only introduced in Python 3.11, so until 3.10 support is dropped we'd have to import Self
from typing-extensions
instead, adding a dependency.
As django-stubs
relies on typing-extensions
already, this dependency must be installed for type checking anyway, but my question to the django-model-utils
maintainers is whether it would be acceptable to have it as a runtime dependency. If not, it is possible to work around it by importing it only when typing.TYPE_CHECKING
is True
, but that will add some noise to the code.
django-stubs
?I filed a handful of PRs on django-stubs
to add or fix support for things that django-model-utils
uses. These have been included in the 5.0 release of django-stubs
.
While some of these changes are needed only internally (for example, by the unit tests), others might be required for the public interface to be accepted by mypy. So in practice, I think we can only rely on django-model-utils
type checking to work with a recent enough version of django-stubs
. How do we communicate this to the end users?
One option would be to include it in the documentation, but this is manual work, which means it's easy to forget to update it, both when making changes to django-model-utils
and at the end user's side when they upgrade django-model-utils
.
What I think would be the proper solution would be to add a dependency extra for type checking that depends on the minimum version of django-stubs
that we know works correctly.
Currently the unit tests use the unittest
class-based approach to organizing test code. In particular the tests/test_fields/test_field_tracker.py
test module uses a complex inheritance structure. While I got that module to be accepted by mypy, the result isn't pretty and liberally uses # type: ignore
overrides. Besides making the tests harder to read, this undermines the purpose of having annotated unit tests, which is to have code that verifies whether the annotations in the public interfaces (the code under test) make sense.
I think that using pytest's native style, with test functions rather than methods, parameter decorators rather than overriding fields, and fixtures to control the setup and teardown might help to clean this up. However, that means converting a lot of test code, the maintainers having to become familiar with a new style and probably adding pytest-django
as a test dependency because some of Django's test helpers only work with unittest.TestCase
out of the box. So that's not something that I should be deciding on my own.
Problem
When type-checking a model that uses
InheritanceManager
usingmypy
, the following error is displayed:According to this Stack Overflow post, the origin of this problem is the
django-stubs
mypy plugin being unable to findInheritanceManager
becausedjango-model-utils
does not provide type annotations.According to the linked post, no actual annotations are required, just a marker that the package is annotated. But I think real annotations would be good to have anyway, both for users and developers of
django-model-utils
.I am working on adding annotations to
django-model-utils
. I don't know if I'll have enough time to completely annotate it, but I'll share what I have in any case, probably tomorrow.Environment
django-stubs
1.16.0