Add type annotations

Question

Add type annotations

mthuurne opened this issue 2 years ago · 33 comments

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

Answer 1 · 2023-03-17T16:56:51.000Z

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 - #559
b2269e7 - #541
df1023b - Remove arguments from InheritanceQuerySetMixin._clone()

Answer 2 · 2023-04-18T17:21:01.000Z

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.

Answer 3 · 2024-03-14T14:52:59.000Z

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

Answer 4 · 2024-03-14T23:56:40.000Z

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

Answer 5 · 2024-03-22T07:30:48.000Z

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

Answer 6 · 2024-03-22T14:00:59.000Z

I'll pick it up again.

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

Answer 7 · 2024-03-22T16:40:56.000Z

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

Answer 8 · 2024-03-22T18:34:07.000Z

Even more cleanups: #597, #598.

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

Answer 9 · 2024-03-22T20:37:23.000Z

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?

Answer 10 · 2024-03-25T11:04:25.000Z

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.

Answer 11 · 2024-03-25T15:36:52.000Z

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.

Answer 12 · 2024-03-25T16:37:03.000Z

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.

Answer 13 · 2024-03-25T18:01:23.000Z

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.

Answer 14 · 2024-03-26T08:07:12.000Z

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?

Answer 15 · 2024-03-26T10:50:07.000Z

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.

Answer 16 · 2024-03-26T10:53:59.000Z

It is behind the dropdown:

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

Answer 17 · 2024-03-26T13:23:38.000Z

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

Answer 18 · 2024-04-04T08:28:43.000Z

With 4.5.0 released we can merge our more impactful changes 🎉

Answer 19 · 2024-04-09T19:43:43.000Z

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.