Opened 3 months ago

Closed 3 months ago

Last modified 2 months ago

#30755 closed New feature (needsinfo)

Have django.views.generic.base.View.as_view() check argument type, via annotations.

Reported by: David Szotten Owned by: nobody
Component: Generic views Version: master
Severity: Normal Keywords: typing
Cc: Triage Stage: Someday/Maybe
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

django.views.generic.base.View.as_view() raises a TypeError if passed a kwarg that's not already an attribute to the class https://github.com/django/django/blame/master/django/views/generic/base.py#L57

From the git blame i can't tell what the reason for this is. Is it only to catch potential mistakes/typos?

If so, would we be open to also accepting attributes in cls.__annotations__?

Change History (9)

comment:1 Changed 3 months ago by Simon Charette

Type: UncategorizedNew feature

I think we should keep the TypeError behaviour as it's an effective way of catching typos and follows the configurable pattern.

If we were adding support fallback for __annotations__ I believe a TypeError should be raised if no value is provided.

Not sure it's worth doing though as foo: annotation = None is a good way to provide a default and an annotation.

comment:2 Changed 3 months ago by David Szotten

the downside of foo: annotation = None is that it doesn't type check: Incompatible types in assignment (expression has type "None", variable has type "str")

comment:3 Changed 3 months ago by Simon Charette

Not sure I understand the issue here, can't you use Optional[str] in this case? Django doesn't support typing yet so I'm not sure of the origin of this type check failure.

comment:4 Changed 3 months ago by Carlton Gibson

Keywords: typing added
Resolution: needsinfo
Status: newclosed
Triage Stage: UnreviewedSomeday/Maybe
Version: 2.2master

Django doesn't support typing yet...

Yeah, I think this is the one.

Type hints seem to have come on a long way in recent versions. The benefits seem to be summing up. So I'm sure they'll come to Django.

But at current state of play, I'd be very surprised if as_view() raised a TypeError here for that.

I'll mark this as Someday/Maybe but close it as needsinfo: all things typing are dependent on a DEP being produced and agreed on. There's a mailing list discussion for that. I'd rather keep everything together there until we have a path forward.

comment:5 Changed 3 months ago by Carlton Gibson

Summary: Arguments to django.views.generic.base.View.as_view()Have django.views.generic.base.View.as_view() check argument type, via annotations.

comment:6 Changed 3 months ago by David Szotten

I'm happy to leave this to the linked mailing list discussion/dep

In the mean time, would you accept a patch pulling out the initkwargs validation from as_view into a helper method, to make it easier for me to override this behaviour in my projects?

Are you able to confirm that the only use-case for the validation is to catch likely mistakes/typos? Not to diminish it, i think it's very valuable, but i'm trying to make sure I'm not breaking something if i override this behaviour. (In a type-annotated codebase I think a type annotation is an equally valid "statement of intent")

For some more context,

the reason I'd rather not make it Optional is that it isn't really and I'd like the type checker to know that this value is always say a str and not make me check for None.

class MyView(View):
    key: str

    def get(self, request):
        # at this point self.key is definitely a str, not an Optional[str]

comment:7 Changed 2 months ago by Carlton Gibson

In the mean time, would you accept a patch pulling out the initkwargs validation from as_view into a helper method, to make it easier for me to override this behaviour in my projects?

TBH, I'd prefer that you override, do your extra validation and then call super(), rather than adding an additional hook, and all that goes with it.

Last edited 2 months ago by Carlton Gibson (previous) (diff)

comment:8 Changed 2 months ago by David Szotten

sorry if i was unclear, but i think part of the original intent got lost. the point is that we also need to _remove_ (well, relax) some of the existing validation, because it checks that all initkwargs are attributes on the class, and key in my example above isn't ("pure" annotations don't become attributes on the class)

comment:9 Changed 2 months ago by Carlton Gibson

Ah, yes. I see. Sorry for not following you. ...

More or less, for me, I'd rather see a actual solution that works for everyone, than a hook to override. (Ultimately bitty APIs don't help anyone...) So I'd rather see effort going into pushing forward the typing efforts.

(In the meantime I'd point you to a mixin or such for your own needs — I know that's not ideal but )

Note: See TracTickets for help on using tickets.
Back to Top