Skip to content

Generic Type Error with ModelAdmin when running in strict mode #507

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

Closed
bradykieffer opened this issue Oct 28, 2020 · 10 comments · Fixed by #526
Closed

Generic Type Error with ModelAdmin when running in strict mode #507

bradykieffer opened this issue Oct 28, 2020 · 10 comments · Fixed by #526
Labels
bug Something isn't working

Comments

@bradykieffer
Copy link

Bug report

What's wrong

I might be crazy here, but when I subclass from admin.ModelAdmin and run mypy with --strict enabled I get an error, error: Missing type parameters for generic type "ModelAdmin". It's completely non-obvious to me how to exactly resolve this. The following code will fail on my machine:

from django.contrib import admin
from my_app import models as db

class FooAdmin(admin.ModelAdmin):
    pass

admin.site.register(db.Foo, FooAdmin)

This code:

class FooAdmin(admin.ModelAdmin[db.Foo]):
    pass

Generates a type error: TypeError: 'MediaDefiningClass' object is not subscriptable

The only way I've been able to resolve this is to do the following:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    class FooAdmin(admin.ModelAdmin[db.Foo]): ...
else:
    class FooAdmin(admin.ModelAdmin):

But that feels like a pretty annoying workaround for all of our admin pages

How is that should be

I believe I should be able to pass in db.Foo when annotating the type for FooAdmin like so: class FooAdmin(admin.ModelAdmin[db.Foo]) although I'm not sure on exactly how to go about this. Any guidance would be much appreciated here

System information

  • OS: macOS
  • python version: 3.8.6
  • django version: 2.2.16
  • mypy version: 0.70
  • django-stubs version: 1.7.0
@bradykieffer bradykieffer added the bug Something isn't working label Oct 28, 2020
@sobolevn
Copy link
Member

We have to patch Django from time to time to fix this 🙂

@bradykieffer
Copy link
Author

@sobolevn just so I understand, you mean submit a patch to django directly for this? Presumably having these classes inherit from Generic at some point right?

@sobolevn
Copy link
Member

Nope, we submit patches to https://github.com/django/django

Example: django/django@578c03b

@bradykieffer
Copy link
Author

Oh awesome, thanks so much. I'll see if I can get a patch in 😄

@jrheard
Copy link
Contributor

jrheard commented Nov 6, 2020

Thanks for creating this ticket and for your work on https://code.djangoproject.com/ticket/32156 , @bradykieffer !

It looks like the Django devs decided not to accept your proposed patch; that's a bummer, but I see where they're coming from. @sobolevn , what do you think should be done next?

Is reverting #504 on the table, or is a better option available? (I'm a drive-by commenter with no insight into that PR or the issue it sought to fix; all I know is that the codebase I work on has ~35 instances of this new error: Missing type parameters for generic type "ModelAdmin" error when updating to django-stubs 1.7.0, and it looks like we're going to have to silence them via one workaround or another.)

@sobolevn
Copy link
Member

sobolevn commented Nov 6, 2020

Well, I agree with Carlton here. Because, it's a really bad idea to ask Django's devs to add __class__getitem__ here and there.
We should not be really dependent on them here. Because of the slow release cycle, long term support, etc.

I think we should go with monkeypatching stuff on our own end. For example, we can add

import django_stubs

django_stubs.make_generics()

in manage.py and do our dirty job.

This way we would hide all the boilerplate away. And won't be dependent on django's implementation.

@bradykieffer
Copy link
Author

bradykieffer commented Nov 6, 2020

@jrheard thanks for the update - it's been a hectic week on my end so I haven't been able to update this. I'm in the same boat as you where we're grappling with how to address this, it would be really nice if django stubs were compatible with supported Django versions for us as well. The approaches that I can see being immediately available are:

  1. Revert make BaseModelAdmin generic to properly type methods dealing with models #504 so that stubs work with supported versions of Django, it stinks that we lose the power of generic but I find it hard to justify using when Django upstream won't accept those patches
  2. Monkey patch django and expose a way to do so via this library, I was thinking of something like an installed Django application for this. I like this idea because it can be optional, doesn't increase dependencies, and tracks where there are divergences in Django's behavior vs the type stubs. If the promise of type checking holds true (that we don't need code that behaves differently at runtime) then this should be doable, but not easy

Not that it's directly related to this but I also had a similar proposal for django-rest-framework passed on here. The motivation is the same, I think until mypy is more widely adopted it will be hard to sell libraries on supporting long term typechecking capabilities even if they're part of the standard library (in their defense, the behavior of type checkers certainly isn't completely stable).

Edit: @sobolevn literally posted long before my above rant, glad we agree

@ghost
Copy link

ghost commented Nov 7, 2020

oh jeez, i didn't even think about the fact that making ModelAdmin generic would actually fail to run. i hate that there's no clean answer here, but i think monkeypatching is absolutely the way to go

@ghost
Copy link

ghost commented Nov 11, 2020

hey @bradykieffer the fix for this just got merged. we ended up going with a sub-project as part of the main repo. you'll have to install it in addition to django stubs with pip install 'git+https://github.com/typeddjango/django-stubs.git#egg=django_stubs_ext&subdirectory=django_stubs_ext'

then somewhere in your top-level urlconf add the following:

import django_stubs_ext.monkeypatch

django_stubs_ext.monkeypatch()

@egregors
Copy link

egregors commented Jan 18, 2021

Hi, @sobolevn! I just got an issue with django_stubs_ext.monkeypatch(). I believe it probably could be a bug. Please, check this out: #557

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

4 participants