[Django] #32156: Add __class_getitem__ to support runtime type parameters for more classes
Django
-------------------------------------+-------------------------------------
Reporter: Brady | Owner: nobody
Kieffer |
Type: New | Status: new
feature |
Component: Utilities | Version: 2.2
Severity: Normal | Keywords: types, mypy,
Triage Stage: | django-stubs
Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
We use the library django-stubs pretty extensively. Recently, they
introduced a patch treating more types as Generics. To handle this on our
side we either need to work `mypy` in a decently annoying manner. Much
like issue #31223, I propose we add a `__class_getitem__` to the following
classes:
* BaseModelAdmin
* Options
* Lookup
* Field
Much like before `__class_getitem__` would purely be for `mypy` type
checks and a no-op for the django runtime. Only other thing to note, for
us the most important class to update would be `BaseModelAdmin`, so if
there are reasons not to add `__class_getitem__` to the other classes, I
can forgo that change.
Related patch - Fixed #31223: https://github.com/django/django/pull/12405
django-stubs: https://github.com/typeddjango/django-stubs
--
Ticket URL: <https://code.djangoproject.com/ticket/32156>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
| Kieffer
Type: New feature | Status: assigned
Component: Utilities | Version: 2.2
Severity: Normal | Resolution:
Keywords: types, mypy, | Triage Stage:
django-stubs | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Brady Kieffer
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32156#comment:1>
Django
-------------------------------------+-------------------------------------
| Kieffer
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: types, mypy, | Triage Stage:
django-stubs | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Hi Brady. Thanks for the input.
I'm a bit sceptical about this. #31223 was fixed on the basis that:
1. ''It'll change our life''
2. It's all we need.
No 2 now (already) seems false.
More though, this is more than a bit boilerplatey...
{{{
@classmethod
def __class_getitem__(cls, *args, **kwargs):
return cls
}}}
Question: why isn't this mypy's issue? i.e. why can't we demand better
type inference in this case?
(Type annotations cool. Unnecessary boilerplate, not so much…)
What do the mypy people say about that type of case?
Thanks for helping me to understand!
--
Ticket URL: <https://code.djangoproject.com/ticket/32156#comment:2>
Django
-------------------------------------+-------------------------------------
| Kieffer
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: types, mypy, | Triage Stage:
django-stubs | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Brady Kieffer):
Hi Carlton,
Thanks for the speedy response. I guess I just want to preface everything
here by saying I pretty squarely fall into the bucket of being a user of
both `Django` + `django-stubs`. I definitely agree that the code is a bit
boilerplate-y. I don't actually think this is a mypy issue, it's more
about how we choose to reconcile the true implementation of Django with
third party libraries attempting to type it in a simple manner. So, when
it comes to:
{{{
@classmethod
def __class_getitem__(cls, *args, **kwargs):
return cls
}}}
I tend to agree that it's pretty annoying that we have to modify Django to
make it usable with `django-stubs`. In defense of `mypy` and typing in
general, there's a more idiomatic approach to this problem by defining a
class as a `Generic` ([https://mypy.readthedocs.io/en/stable/generics.html
#defining-generic-classes docs for Generics here]). I _believe_ the reason
that adding `Generic` as a base for `QuerySet` + `Manager` was avoided
because it could have an impact downstream but, I don't really know why
this was decided. So, `mypy` would recommend we use `Generic`, which we
likely will not, and it sort of leaves open the question of what should we
reasonably expect to do with Django **if** we want to support third party
typing libraries. I'm not super opinionated, but I would definitely rather
not have to monkeypatch every codebase that uses django-stubs if it can be
avoided. If we do go down this path though, I would agree that your second
point will likely never be satisfied.
Type checking is really cool, and it's saved us from some very weird bugs.
But, it definitely feels like we're in a weird middle ground where there
isn't "the one perfect way" of doing it.
--
Ticket URL: <https://code.djangoproject.com/ticket/32156#comment:3>
Django
-------------------------------------+-------------------------------------
| Kieffer
Component: Utilities | Version: 2.2
Severity: Normal | Resolution: needsinfo
Keywords: types, mypy, | Triage Stage:
django-stubs | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => needsinfo
Comment:
Hi Brady. Thanks for the follow-up.
> In defense of mypy and typing in general, there's a more idiomatic
approach to this problem by defining a class as a Generic (docs for
Generics here).
First: Not criticising you here! :)
This thought seems to me to have lost the wood for the trees. Translated
it reads: Add a lot of noise to your code in order to avoid adding a lot
of (more boilerplate) noise to your code. Typing was meant to be an
optional add-on to Python. I can't (yet) see that it's not an issue with
mypy that we have to add this stuff when (surely) `cls` could be inferred
as the default for `__class_getitem__`. (Truly this baffles me.)
Don't suppose you know the best place to make this kind of point to the
mypy maintainers do you? (I'll have to look around.)
I'm going to have to close this as needsinfo. I can't see that accepting
this here is compatible with the previous Technical Board decision. I
think we need to go back to the DevelopersMailingList to ask for review.
--
Ticket URL: <https://code.djangoproject.com/ticket/32156#comment:4>
Django
-------------------------------------+-------------------------------------
| Kieffer
Type: New feature | Status: closed
Severity: Normal | Resolution: needsinfo
Keywords: types, mypy, | Triage Stage:
django-stubs | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Hi Brady.
I was following up with `mypy` folks here, trying to workout if there
really is no option beyond the trivial `__class_getitem__` implementation.
It was asked whether `from __future__ import annotations`
[https://www.python.org/dev/peps/pep-0563/ as per PEP 563] would stop the
interpreter error.
Can you comment on that?
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/32156#comment:5>
Django
-------------------------------------+-------------------------------------
| Kieffer
Component: Utilities | Version: 2.2
Severity: Normal | Resolution:
Keywords: types, mypy, | Triage Stage:
django-stubs | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: needsinfo =>
Comment:
Hi Carlton,
I didn't think you were criticizing, I actually agree with your worry
about the boilerplate :)
As for the `from __future__ import annotations` suggestion - that won't
actually fix the issue in this case. The problem is that we're
encountering a type error for the following snippet of code:
{{{
from django.contrb import admin
from myapp import models as db
class MyModelAdmin(admin.ModelAdmin):
pass
admin.site.register(db.MyModel, MyModelAdmin)
}}}
In `django-stubs 1.7.0` `BaseModelAdmin` was made generic to allow
properly typing model admin methods that expect a model class of a certain
type, [https://github.com/typeddjango/django-stubs/pull/504 link to this
patch]. Due to this change, when running mypy in strict mode, the above
snippet won't work because `admin.ModelAdmin` needs a type parameter. So,
we'd need to write the following code:
{{{
from django.contrb import admin
from myapp import models as db
class MyModelAdmin(admin.ModelAdmin[db.MyModel]):
pass
admin.site.register(db.MyModel, MyModelAdmin)
}}}
which breaks because `admin.ModelAdmin` does not have the
`__class_getitem__` attribute. So, to fix this issue I ''think'' the paths
forward would be one of the following:
1. Have `BaseModelAdmin` inherit directly from `Generic`, I ''think'' this
is the most correct approach but I'm not 100% certain (getting input from
the `mypy` devs here would be a good idea). This would be the least
boilerplate-y and `Generic` was added in Python 3.5, so this should be
fine for all supported versions of Django. The only downside I can think
of with this approach is if there would be some weirdness with having
`Generic` for all of these classes? I don't really know
2. Directly bolt on a no-op for `__class_getitem__` to the
`BaseModelAdmin` class (as I did in the proposed patch), which would allow
me to write `class MyModelAdmin(admin.ModelAdmin[db.MyModel]): ...` . This
directly solves the issue at hand but is boilerplate-y and doesn't address
the underlying problem going forward.
3. Revert the changes in `django-stubs` to account for the fact that this
functionality isn't yet supported in Django (I'm not a fan of this one).
Would stop typechecking from breaking for us but we'd lose out on a lot of
powerful typing functionality, something I think django-stubs has done
very well.
4. Monkey-patch `BaseModelAdmin` directly. I'm not a fan of this approach
either because it feels like we'd just be kicking the can down the road.
Also, it'll be more work for my team to maintain divergent behavior.
Although, the implementation really isn't difficult, it could just be done
like so:
{{{
from django.contrib.admin import options
options.BaseModelAdmin.__class_getitem__ = lambda cls, *args, **kwargs:
cls # type: ignore
}}}
I'm personally a fan of (1) because it avoids the issue of boilerplate
entirely + leverages builtin types.
Refs:
django stubs: https://github.com/typeddjango/django-stubs
mypy generics:
https://mypy.readthedocs.io/en/stable/generics.html#generics
django stubs issue related to this: https://github.com/typeddjango/django-
stubs/issues/507
--
Ticket URL: <https://code.djangoproject.com/ticket/32156#comment:6>
Django
-------------------------------------+-------------------------------------
| Kieffer
Component: Utilities | Version: 2.2
Severity: Normal | Resolution: wontfix
Keywords: types, mypy, | Triage Stage:
django-stubs | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
Hi Brady. Thanks for the follow-up.
Given the previous discussion, this is a wontfix issue. The correct
workflow to change that is to go via the DevelopersMailingList (see
TicketClosingReasons/DontReopenTickets)
Of your options, I'll just say here, that:
* I worry about 1 — We're enforcing a structure that's never been there
and never been needed. Typing is meant to serve the code, not the other
way round. (I'm not convinced that ModelAdmin is best modelled as a
statically typed generic...)
* 4 actually seems like a good option here. This stuff is still evolving.
If we change Django now and find it goes a different way, we'll be stuck.
What do the core devs say on this kind of example? Typing is meant to be
optional, an add-on. Surely the goal is to be able to work with code such
as Django's without the requirement that we retrofit all this stuff?
The promise of stubs was to keep the typing stuff out of the code: it
seems like a valid bug report on mypy or typing that "We can't implement
X, Y, Z in django-stubs without requiring change A, B, C in Django." — If
you make that kind of point, what response do you get?
Thanks again.
--
Ticket URL: <https://code.djangoproject.com/ticket/32156#comment:7>
Django
-------------------------------------+-------------------------------------
| Kieffer
Type: New feature | Status: closed
Severity: Normal | Resolution: wontfix
Keywords: types, mypy, | Triage Stage:
django-stubs | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam (Chainz) Johnson):
Just to add my 2c - I think option 4, having django-stubs monkey-patch
django, is a good approach. "Kicking the can down the road" is probably a
good approach whilst things are still evolving.
--
Ticket URL: <https://code.djangoproject.com/ticket/32156#comment:8>