Incorrect annotation for ModelAdmin.view_on_site

[cuu508]

Bug report

What's wrong

I have a ModelAdmin that has a view_on_site method similar to the example in Django docs:

class PersonAdmin(admin.ModelAdmin):
    def view_on_site(self, obj):
        url = reverse("person-detail", kwargs={"slug": obj.slug})
        return "https://example.com" + url

If I annotate it like so:

class PersonAdmin(admin.ModelAdmin[Person]):
    def view_on_site(self, obj: Person) -> str:
        url = reverse("person-detail", kwargs={"slug": obj.slug})
        return "https://example.com" + url

I get a type warning:

error: Signature of "view_on_site" incompatible with supertype "BaseModelAdmin"  [override]
note:      Superclass:
note:          bool | Callable[[_ModelT], str]
note:      Subclass:
note:          def view_on_site(self, obj: Person) -> str

How is that should be

It should not generate the warning. I don't know how to fix that though :-/

System information

• OS:
• python version: 3.10.12
• django version: 4.2.5
• mypy version: 1.5.1
• django-stubs version: 4.2.4
• django-stubs-ext version: 4.2.2

[apollo13]

I have pushed a partial fix to healthchecks/healthchecks#904 for healthchecks.io. That shows a way on how to fix the reported error, it doesn't fix it for hc though :/

[apollo13]

I feel like that the typing is wrong in django-stubs here, the callable doesn't take self?

[apollo13]

Okay, the missing self alone is not the sole issue. Another issue is probably also that the override does not support bool as option (which makes sense since it narrows the type and Django explicitly checks for that). I wonder if a typing: ignore comment would be the best choice here :)

[GabDug]

Have the same issue, using a Protocol seems to do the trick, unless I missed something. If you or anyone else is willing to do a PR, go ahead, else I'll try to get to it at some point :)

# django-stubs/contrib/admin/options.pyi
_ModelT = TypeVar("_ModelT", bound=Model)

class ViewOnSiteProtocol(Protocol, Generic[_ModelT]):
    def __call__(self, obj: _ModelT) -> str: ...

ViewOnSiteAlias: TypeAlias = Union[ViewOnSiteProtocol[_ModelT], bool]
"Either a boolean or a callable that takes a model instance and returns a URL. Callable can be static."

class BaseModelAdmin(Generic[_ModelT]):
    ...
    view_on_site: ViewOnSiteAlias

# Examples
class TestStaticAdmin(admin.ModelAdmin[Test]):
    ...
    @staticmethod
    def view_on_site(obj: Test) -> str:
        return obj.web_url

class TestAdmin(admin.ModelAdmin[Test]):
    ...
    def view_on_site(obj: PagerDutyIncident) -> str:
        return obj.web_url

class TestLambdaAdmin(admin.ModelAdmin[Test]):
    ...
    view_on_site = lambda obj: obj.web_url

class TestBoolAdmin(admin.ModelAdmin[Test]):
    ...
    view_on_site = True

[flaeppe]

# django-stubs/contrib/admin/options.pyi
_ModelT = TypeVar("_ModelT", bound=Model)

class ViewOnSiteProtocol(Protocol, Generic[_ModelT]):
    def __call__(self, obj: _ModelT) -> str: ...

ViewOnSiteAlias: TypeAlias = Union[ViewOnSiteProtocol[_ModelT], bool]
"Either a boolean or a callable that takes a model instance and returns a URL. Callable can be static."

class BaseModelAdmin(Generic[_ModelT]):
    ...
    view_on_site: ViewOnSiteAlias

I don't think the ViewOnSiteAlias: TypeAlias = Union[ViewOnSiteProtocol[_ModelT], bool] is bound to _ModelT here while it might still look like it.

Scoping wise, the generics of ViewOnSiteProtocol can't be in a type aliased. It needs to go under the class scope to get the same bound as the generics of BaseModelAdmin.

e.g.

# django-stubs/contrib/admin/options.pyi
_ModelT = TypeVar("_ModelT", bound=Model)

class ViewOnSiteProtocol(Protocol, Generic[_ModelT]):
    def __call__(self, obj: _ModelT) -> str: ...

-ViewOnSiteAlias: TypeAlias = Union[ViewOnSiteProtocol[_ModelT], bool]
"Either a boolean or a callable that takes a model instance and returns a URL. Callable can be static."

class BaseModelAdmin(Generic[_ModelT]):
    ...
-    view_on_site: ViewOnSiteAlias
+    view_on_site: ViewOnSiteProtocol[_ModelT] | bool

[UnknownPlatypus]

I tried not so successfully to implement your ideas but it seems really hard to make mypy understand something could be a boolean or a bound method (which kinda makes sense, this is not an usual pattern). I couldn't get rid of this error:

E     main:13: error: Signature of "view_on_site" incompatible with supertype "BaseModelAdmin"  [override] (diff)
E     main:13: note:      Superclass:               (diff)
E     main:13: note:          Union[ViewOnSiteProtocol[_ModelT], bool] (diff)
E     main:13: note:      Subclass:                 (diff)
E     main:13: note:          def view_on_site(self, obj: TestModel) -> str (diff)

I used this test case and snippet if someone wants to give it a try too:

-   case: test_view_on_site
    main: |
        from django.contrib.admin import ModelAdmin
        from django.db import models

        class TestModel(models.Model):
            web_url = "a"

        class TestStaticAdmin(ModelAdmin[TestModel]):
            @staticmethod
            def view_on_site(obj: TestModel) -> str:
                return obj.web_url

        class TestAdmin(ModelAdmin[TestModel]):
            def view_on_site(self, obj: TestModel) -> str:
                return obj.web_url

        class TestLambdaAdmin(ModelAdmin[TestModel]):
            view_on_site = lambda obj: obj.web_url

        class TestBoolAdmin(ModelAdmin[TestModel]):
            view_on_site = True

_ModelT_contra = TypeVar("_ModelT_contra", bound=Model, contravariant=True)

class ViewOnSiteProtocol(Protocol, Generic[_ModelT_contra]):
    def __call__(self, obj: _ModelT_contra) -> str: ...

[cuu508]

I asked about this in python/typing discussions and got an interesting answer: python/typing#1648 (comment)

I could adapt the idea for view_on_site like so:

# instead of 
view_on_site: Callable[[_ModelT], str] | bool

# do
@property
def view_on_site(self) -> Callable[[_ModelT], str] | bool:
    raise NotImplementedError

At least in my case this does indeed satisfy mypy! My only concern is this feels like working around a quirk in mypy. An admin subclass could in theory change the field's value at runtime. Not sure if there are sensible reasons to do so, but it could, I think.

[sobolevn]

PR is welcome :)