Improve types of django.contrib.admin.decorators

[flaeppe]

I have made things!

• Type annotations of django.contrib.admin.decorators previously changed the signature of the classes/functions that were decorated. Not any more.
• The decorators now set a bound on the signature of what they support decorating. As such an error will be emitted on an incorrect signature of the decorated function/class.
  • Take note that since a bound is used, annotating subclasses of the bounds are supported.
• The description= arguments now supports both Promise and str -> _StrPromise (for gettext_lazy support)

Related issues

None

[intgr]

Would be easier and less repetition to define

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

[flaeppe]

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

[flaeppe]

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

[flaeppe]

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

[flaeppe]

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🙂)

[flaeppe]

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

[intgr]

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.

[flaeppe]

[...] 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.

[intgr]

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.

[flaeppe]

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

[intgr]

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.

[flaeppe]

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

[intgr]

Sorry to delay this for so long.

Since this change is fairly complex, I'd like to see a test case for these as well. Look in tests/typecheck/ for examples. A single ModelAdmin class with methods where each decorator is used with @bare as well as @with(args) calling style would be great.

With that, I'm happy to approve this.

[intgr]

Thanks! I played around with this and created some test cases.

I'm going to merge this and open another PR with the tests and some additional tweaks.

[flaeppe]

Sorry for being so slow, I've had a couple of test cases locally since I built this. Just never came around to pushing it

[intgr]

Sorry if I was too impatient. Maybe some advance heads-up would have been warranted.

If you don't mind, have a look at #1292 and see if I missed anything.

[flaeppe]

Not to worry, it's me being slow. I'll try to have a look tomorrow