Opened 4 years ago

Closed 4 years ago

Last modified 4 years 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: dev
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 by Simon Charette, 4 years ago

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 by David Szotten, 4 years ago

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 by Simon Charette, 4 years ago

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 by Carlton Gibson, 4 years ago

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 by Carlton Gibson, 4 years ago

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 by David Szotten, 4 years ago

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 by Carlton Gibson, 4 years ago

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 4 years ago by Carlton Gibson (previous) (diff)

comment:8 by David Szotten, 4 years ago

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 by Carlton Gibson, 4 years ago

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