Skip to content

Improve types of django.contrib.admin.decorators #1267

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 13, 2022
Merged

Improve types of django.contrib.admin.decorators #1267

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 32 additions & 9 deletions django-stubs/contrib/admin/decorators.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,50 @@
from collections.abc import Callable, Sequence
from typing import Any, TypeVar
from typing import Any, TypeVar, overload

from django.contrib.admin import ModelAdmin
from django.contrib.admin.sites import AdminSite
from django.db.models import Combinable, QuerySet
from django.db.models.base import Model
from django.db.models.expressions import BaseExpression
from django.http import HttpRequest
from django.utils.functional import _StrPromise

_ModelT = TypeVar("_ModelT", bound=Model)
_T = TypeVar("_T")
_Model = TypeVar("_Model", bound=Model)
_ModelAdmin = TypeVar("_ModelAdmin", bound=ModelAdmin)
_Request = TypeVar("_Request", bound=HttpRequest)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only remaining dilemma I have is whether the request argument should be a TypeVar, or change it to just HttpRequest without TypeVar.

It seems most django-stubs code already assumes that requests are always HttpRequest and that tends to be sufficient.

Technically most of the time they're WSGIRequest, but that seems to be an implementation detail. There is no explicit documentation about WSGIRequest in Django docs (barring testing topics and release notes). And ASGIRequest is now now rearing its head.

Unless someone else chimes in, I'll trust your judgment here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the type-unsafety you've already stated still stands. Having this, it should be possible for someone to do

@action(...)
def my_action(..., request: MyCustomHttpRequest, ...) -> None:
    request.attribute_set_by_middleware

e.g. Capturing attributes attached by middleware(s) or so.

Or doing stuff mentioned in docs: https://github.com/typeddjango/django-stubs#how-can-i-create-a-httprequest-thats-guaranteed-to-have-an-authenticated-user

_QuerySet = TypeVar("_QuerySet", bound=QuerySet)

@overload
def action(
function: Callable[[_ModelAdmin, _Request, _QuerySet], None],
permissions: Sequence[str] | None = ...,
description: _StrPromise | None = ...,
) -> Callable[[_ModelAdmin, _Request, _QuerySet], None]: ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be easier and less repetition to define

_ActionCallable = TypeVar("_ActionCallable", bound=Callable[[ModelAdmin, HttpRequest, QuerySet], None])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not gonna work. It needs a bound, if you declare them without a TypeVar it's gonna change the signature of the function that is decorated

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right, the bound callable won't match either. I started out with that but then you'll need nested TypeVars to get a correct bound. Else we're not supporting subclasses. See here

https://mypy-play.net/?mypy=0.982&python=3.10&gist=de1239d0a4ced1707ec6afb9f4610160

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks cluttered, I agree, but this was the only way I could get it to work correctly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you manage to figure out how to declare a TypeVar(..., bound=Callable[...]) that works.
Pass me a playground link showing how, I'm very interested in knowing how to with mypy (I want to use it in other projects🙂)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://mypy-play.net/?mypy=0.990&python=3.11&gist=5670bdd2e498f5159d47e26a41d653d5

Copy link
Collaborator

@intgr intgr Nov 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I understand now. Ordinarily with bound=Callable[[Model], ...], the type system applies contravariant type compatibility to arguments of the callable: to be type-safe, the callable must accept every possible Model object that could be passed in. (explanation)

What you're trying to do here is covariant behavior: the callable should accept some subclass of Model as argument. A TypeVar is bound checking is covariant. Which is the correct subclass in the particular instance won't be type-checked.

It's a bit of a hack and not entirely type-safe, but I think practicality beats purity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] Which is the correct subclass in the particular instance won't be type-checked.

It's a bit of a hack and not entirely type-safe, but I think practicality beats purity.

Could you elaborate on what it is that won't be type checked and why it's not type safe? (I presume with the current display annotations func: Callable[[_ModelT], _T])

I'm not sure I'm following on how that conclusion is reached.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example given two models Foo and Bar. With your proposed solution, you may have a FooAdmin class with a @display method def my_display_func(obj: Bar). The type checker has no idea that Bar is incorrect and the method will actually accept Foo instances. That's what I meant by "not type-safe"

The "type-safe" approach is contravariant compatibility: you can define my_display_func(obj: Model) or my_display_func(obj: object), and it's assured that any passed argument is always an instance of Model or object. It's type-safe, but useless: almost every display callback needs to access fields of the particular Model subclass, you'd need to use cast() or isinstance() guards to do most useful things.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah that's true. Didn't think of that.

I suppose in order to get around that, it would be necessary for admin.display to have the context of FooAdmin (e.g. FooAdmin.model), although it's a naive, dynamic, decorator that doesn't even consider the ModelAdmin.

What I'm saying is e.g. @admin.display[MyModelAdmin] (if ModelAdmin accepted arguments runtime) and pick up its model. But then it also has to be possible to pick up the Model argument for ModelAdmin

@overload
def action(
function: Callable[[ModelAdmin, HttpRequest, QuerySet], None] | None = ...,
*,
permissions: Sequence[str] | None = ...,
description: str | None = ...,
) -> Callable: ...
description: _StrPromise | None = ...,
) -> Callable[
[Callable[[_ModelAdmin, _Request, _QuerySet], None]], Callable[[_ModelAdmin, _Request, _QuerySet], None]
]: ...
@overload
def display(
function: Callable[[_Model], _T],
boolean: bool | None = ...,
ordering: str | Combinable | BaseExpression | None = ...,
description: _StrPromise | None = ...,
empty_value: str | None = ...,
) -> Callable[[_Model], _T]: ...
@overload
def display(
function: Callable[[_ModelT], Any] | None = ...,
*,
boolean: bool | None = ...,
ordering: str | Combinable | BaseExpression | None = ...,
description: str | None = ...,
description: _StrPromise | None = ...,
empty_value: str | None = ...,
) -> Callable: ...
def register(*models: type[Model], site: AdminSite | None = ...) -> Callable: ...
) -> Callable[[Callable[[_Model], _T]], Callable[[_Model], _T]]: ...
def register(
*models: type[Model], site: AdminSite | None = ...
) -> Callable[[type[_ModelAdmin]], type[_ModelAdmin]]: ...