Add type annotations

[mthuurne]

Problem

When type-checking a model that uses InheritanceManager using mypy, the following error is displayed:

app/models.py:39: error: Could not resolve manager type for
"app.models.MyModel.objects"  [django-manager-missing]
        objects = InheritanceManager()
        ^

According to this Stack Overflow post, the origin of this problem is the django-stubs mypy plugin being unable to find InheritanceManager because django-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 Model Utils version: 4.3.1
• Django version: 3.2
• Python version: 3.9
• Other libraries used, if any: django-stubs 1.16.0

[mthuurne]

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:

• b656453 - Update where ConnectionDoesNotExist is imported from
• 30de857 - FieldInstanceTracker passes str instead of Field to DeferredAttributeTracker #559
• b2269e7 - delete() method don't returns number of affected rows #541
• df1023b - Remove arguments from InheritanceQuerySetMixin._clone()

[mthuurne]

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.

[Mostaard]

Hi! I was wondering whether you have made any progress on this? I would like to use types too. Can I help?

[mthuurne]

I haven't made progress since last year. I'm still waiting for my cleanups PRs #566 and #567 to be reviewed.

[foarsitter]

@mthuurne both PR's you are mentioning are merged. Are you willing to take the further lead on typing?

[mthuurne]

I'll pick it up again.

I split off one cleanup that can already be applied: #591.

[mthuurne]

More cleanups: #594, #595, #596.

[mthuurne]

Even more cleanups: #597, #598.

I also updated the branch on top of the latest master.

[foarsitter]

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?

[mthuurne]

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.

[foarsitter]

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.

[mthuurne]

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.

[mthuurne]

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.

[foarsitter]

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?

[mthuurne]

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.

[foarsitter]

It is behind the dropdown:

Thanks for your draft, a lot to learn for me about typing!

[mthuurne]

Here is a new series of cleanups that can be merged separately: #604, #605, #606, #607.

[foarsitter]

With 4.5.0 released we can merge our more impactful changes 🎉

[mthuurne]

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.

[foarsitter]

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.

[mthuurne]

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 869c6a6, 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 80dda9f.

What would be the preferred way forward:

• duplicate the signatures for all used assertX() methods in each mixin?
• use the fake base class approach for all mixins?
• convert to pytest-style assertions everywhere?
• convert to pytest-style assertions only where needed?

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.

[mthuurne]

Another small cleanup: #611. This one is backwards incompatible, but that means now is a good time to do that, right?

[foarsitter]

Yes, 5.0 will be full of breaking changes :)

[mthuurne]

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.

[mthuurne]

More cleanups: #612, #613, #614

[mthuurne]

The draft PR #603 passed CI checks for the first time.

There is still work to do though:

• two errors are reported when the mypy cache isn't empty; I suspect this is an issue in the django-stubs mypy plugin
• add py.typed marker and start checking applications against the annotated library
• check for type arguments that are not forwarded properly, like model type falling back to Any
• 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

[foarsitter]

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

[gmcrocetti]

Do you guys believe an extra pair of hands would help in closing this issue and releasing 5.x ? @foarsitter @mthuurne

[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:

• install the code from Add type annotations #603 and use mypy to type check your own application that uses django-model-utils against it
• review the type annotations: check whether they make sense and whether there might be a simpler way to achieve the same effect

[mthuurne]

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

[foarsitter]

If we are in the testing phase I would like to release a beta, what do you people think?

[gmcrocetti]

I agree @foarsitter , let's make a release candidate 🚀 .