Skip to content
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

Allow returning Response from admin @action callbacks #1331

Merged
Merged
12 changes: 6 additions & 6 deletions django-stubs/contrib/admin/decorators.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ 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.http import HttpRequest, HttpResponseBase
from django.utils.functional import _StrOrPromise
from typing_extensions import TypeAlias

Expand All @@ -17,21 +17,21 @@ _QuerySet = TypeVar("_QuerySet", bound=QuerySet)
# This is deliberately different from _DisplayT defined in contrib.admin.options
_DisplayCallable: TypeAlias = Union[Callable[[_ModelAdmin, _Model], Any], Callable[[_Model], Any]] # noqa: Y037
_DisplayCallableT = TypeVar("_DisplayCallableT", bound=_DisplayCallable)
_ActionCallable: TypeAlias = Callable[[_ModelAdmin, _Request, _QuerySet], HttpResponseBase | None] # noqa: Y037
_ActionCallableT = TypeVar("_ActionCallableT", bound=_ActionCallable)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there was a good reason why these Callable[...] were duplicated before at each overload.

I'm not sure why exactly -- maybe mypy doesn't support TypeVars inside TypeVars -- but after your changes, this no longer raises an error with mypy at the location of the decorator:

    @admin.action(description="Some text here", permissions=["test"])
    def freestanding_action_fancy(modeladmin: int, request: int, queryset: int) -> None: ...

I suppose this logic could also use a few negative test cases -- checking that mypy correctly flags incorrect usage of the parameters or return types.

Copy link
Member

@flaeppe flaeppe Jan 23, 2023

Choose a reason for hiding this comment

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

Yes, TypeVar can't be bound to a TypeAlias, it'll resolve to Any before it's matched with generic arguments of a class or a callable.

A few non-happy test cases should make that obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I've reverted the previous change and added more tests to highlight this issue. I've left _DisplayCallableT alone, but I'd imagine it probably has a similar problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see… Yes, if it's a problem, then the equivalent for display is also surely broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder whether it'd be worthwhile to enable disallow_any_generics for this repo -- I put together a quick repro, and it looks like if we enable the rule we'll get warnings for this: https://mypy-play.net/?mypy=latest&python=3.11&flags=disallow-any-generics&gist=bf198a38181d36c2105ce3fef468d702

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, maybe, but best looked into separately later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm afraid the fallout from disallow_any_generics will be enormous. But I may be wrong :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found 630 errors in 155 files (checked 675 source files)

You aren't wrong. 😬 Here's a quick stab at it though: #1339


@overload
def action(
function: Callable[[_ModelAdmin, _Request, _QuerySet], None],
function: _ActionCallableT,
permissions: Sequence[str] | None = ...,
description: _StrOrPromise | None = ...,
) -> Callable[[_ModelAdmin, _Request, _QuerySet], None]: ...
) -> _ActionCallableT: ...
@overload
def action(
*,
permissions: Sequence[str] | None = ...,
description: _StrOrPromise | None = ...,
) -> Callable[
[Callable[[_ModelAdmin, _Request, _QuerySet], None]], Callable[[_ModelAdmin, _Request, _QuerySet], None]
]: ...
) -> Callable[[_ActionCallableT], _ActionCallableT,]: ...
christianbundy marked this conversation as resolved.
Show resolved Hide resolved
@overload
def display(
function: _DisplayCallableT,
Expand Down
4 changes: 2 additions & 2 deletions django-stubs/contrib/admin/options.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ from django.forms.models import (
)
from django.forms.widgets import Media
from django.http.request import HttpRequest
from django.http.response import HttpResponse, HttpResponseRedirect
from django.http.response import HttpResponse, HttpResponseBase, HttpResponseRedirect
from django.template.response import _TemplateForResponseT
from django.urls.resolvers import URLPattern
from django.utils.datastructures import _ListOrTuple
Expand Down Expand Up @@ -131,7 +131,7 @@ class BaseModelAdmin(Generic[_ModelT]):

_DisplayT: TypeAlias = _ListOrTuple[str | Callable[[_ModelT], str | bool]]
_ModelAdmin = TypeVar("_ModelAdmin", bound=ModelAdmin)
_ActionCallable: TypeAlias = Callable[[_ModelAdmin, HttpRequest, QuerySet[_ModelT]], None]
_ActionCallable: TypeAlias = Callable[[_ModelAdmin, HttpRequest, QuerySet[_ModelT]], Optional[HttpResponseBase]]
christianbundy marked this conversation as resolved.
Show resolved Hide resolved

class ModelAdmin(BaseModelAdmin[_ModelT]):
list_display: _DisplayT
Expand Down
19 changes: 16 additions & 3 deletions tests/typecheck/contrib/admin/test_decorators.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@

from django.contrib import admin
from django.db.models import QuerySet
from django.http import HttpRequest
from django.http import FileResponse, HttpRequest, HttpResponse


class MyModel(models.Model): ...
Expand All @@ -73,6 +73,11 @@
@admin.action(description="Some text here", permissions=["test"])
def freestanding_action_fancy(modeladmin: "MyModelAdmin", request: HttpRequest, queryset: QuerySet[MyModel]) -> None: ...

@admin.action
def freestanding_action_http_response(modeladmin: "MyModelAdmin", request: HttpRequest, queryset: QuerySet[MyModel]) -> HttpResponse: ...

@admin.action
def freestanding_action_file_response(modeladmin: "MyModelAdmin", request: HttpRequest, queryset: QuerySet[MyModel]) -> FileResponse: ...
christianbundy marked this conversation as resolved.
Show resolved Hide resolved

@admin.register(MyModel)
class MyModelAdmin(admin.ModelAdmin[MyModel]):
Expand All @@ -84,6 +89,14 @@
@admin.action(description="Some text here", permissions=["test"])
def method_action_fancy(self, request: HttpRequest, queryset: QuerySet[MyModel]) -> None: ...

@admin.action(description="Some text here", permissions=["test"])
def method_action_http_response(self, request: HttpRequest, queryset: QuerySet[MyModel]) -> HttpResponse: ...

@admin.action(description="Some text here", permissions=["test"])
def method_action_file_response(self, request: HttpRequest, queryset: QuerySet[MyModel]) -> FileResponse: ...

def method(self) -> None:
reveal_type(self.method_action_bare) # N: Revealed type is "def (django.http.request.HttpRequest, django.db.models.query._QuerySet[main.MyModel, main.MyModel])"
reveal_type(self.method_action_fancy) # N: Revealed type is "def (django.http.request.HttpRequest, django.db.models.query._QuerySet[main.MyModel, main.MyModel])"
reveal_type(self.method_action_bare) # N: Revealed type is "def (request: django.http.request.HttpRequest, queryset: django.db.models.query._QuerySet[main.MyModel, main.MyModel])"
reveal_type(self.method_action_fancy) # N: Revealed type is "def (request: django.http.request.HttpRequest, queryset: django.db.models.query._QuerySet[main.MyModel, main.MyModel])"
reveal_type(self.method_action_http_response) # N: Revealed type is "def (request: django.http.request.HttpRequest, queryset: django.db.models.query._QuerySet[main.MyModel, main.MyModel]) -> django.http.response.HttpResponse"
reveal_type(self.method_action_file_response) # N: Revealed type is "def (request: django.http.request.HttpRequest, queryset: django.db.models.query._QuerySet[main.MyModel, main.MyModel]) -> django.http.response.FileResponse"
2 changes: 1 addition & 1 deletion tests/typecheck/contrib/admin/test_options.yml
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@
pass

class A(admin.ModelAdmin):
actions = [an_action] # E: List item 0 has incompatible type "Callable[[None], None]"; expected "Union[Callable[[Any, HttpRequest, _QuerySet[Any, Any]], None], str]"
actions = [an_action] # E: List item 0 has incompatible type "Callable[[None], None]"; expected "Union[Callable[[Any, HttpRequest, _QuerySet[Any, Any]], Optional[HttpResponseBase]], str]"
- case: errors_for_invalid_model_admin_generic
main: |
from django.contrib.admin import ModelAdmin
Expand Down