Opened 9 years ago

Closed 9 years ago

#25138 closed New feature (wontfix)

Easier decoration of class-based views

Reported by: George Brocklehurst Owned by: nobody
Component: Generic views Version: 1.8
Severity: Normal Keywords: views decorators
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The class-based view documentation (1) recommends:

To decorate every instance of a class-based view, you need to decorate the class definition itself. To do this you apply the decorator to the dispatch() method of the class.

This can get a little syntactically cumbersome:

class ProtectedView(generic.TemplateView):
    template_name = 'secret.html'
 
    @method_decorator(login_required)
    def dispatch(self, *args, **kwargs):
        return super(ProtectedView, self).dispatch(*args, **kwargs)

Instead, we could provide a decorator that can be applied to the view class:

@dispatch_decorator(login_required)
class ProtectedView(generic.TemplateView):
    template_name = 'secret.html'

I've spiked on a quick implementation of this, which seems to work (2). I'm happy to write that up as a proper PR with tests, if it seems like it would be useful.

(1): https://docs.djangoproject.com/en/1.8/topics/class-based-views/intro/#decorating-the-class
(2): https://gist.github.com/georgebrock/24b831c65b09b309c618

Change History (6)

comment:2 by Andriy Sokolovskiy, 9 years ago

I don't see much benefit adding a new decorator which is using existing one.
Actually, decorating a class to decorate a method inside it is non-obvious, it is better to write things
more explicitly.

You can always write own decorator and use it if you need to write such logic, there is no need to adding more and more things to django which can be simply resolved writing few lines of own code.

comment:3 by George Brocklehurst, 9 years ago

Actually, decorating a class to decorate a method inside it is non-obvious, it is better to write things more explicitly.

I partly agree with you: this decorator introduces an abstraction which hides the mechanism of how a function-decorator is being applied to a class-based view.

I don't think that's the most important thing here, though. The idea we want to express in our projects' code is something like “this view requires login” (via login_required) or “this view is wrapped in a middleware” (via decorator_from_middleware), or more abstractly “this view is decorated”. The decorator I'm proposing lets us express that idea more directly and concisely to the next person who needs to read our code, making the code more readable. Depending on how we define “explicit” this change might be considered less explicit—because the mechanism is less clear—or more explicit—because the intent is more clear.

If there's a direct trade-off to be made between clarity of mechanism and clarity of intent, my vote would usually be for clarity of intent. Especially at the boundary between framework code and project code.

You can always write own decorator and use it if you need to write such logic, there is no need to adding more and more things to django which can be simply resolved writing few lines of own code.

I think these few lines of code are useful enough that I'd use them in every project, so I don't want to have to duplicate them. But we absolutely need to draw that line somewhere, and maybe this change is on the wrong side of it.

comment:4 by Markus Holtermann, 9 years ago

When you refer to the login_required decorator, you should use the respective mixin from 1.9 upwards: https://docs.djangoproject.com/en/dev/releases/1.9/#permission-mixins-for-class-based-views

If you have code that is used across your views, move it to mixins and inherit from them. That's the intention of CBVs.

comment:5 by Tim Graham, 9 years ago

An alternate idea could be to allow @method_decorator to work at the class level. Something like @method_decorator(login_required, name='dispatch'). That solves the issue of having to redeclare the decorated method even if no other customizations are needed. I think something more generic that like could be suitable for inclusion in Django.

comment:6 by George Brocklehurst, 9 years ago

Resolution: wontfix
Status: newclosed

When you refer to the login_required decorator, you should use the respective mixin from 1.9 upwards

Oh, that's excellent. I didn't know about the new mixins.

An alternate idea could be to allow @method_decorator to work at the class level. Something like @method_decorator(login_required, name='dispatch'). That solves the issue of having to redeclare the decorated method even if no other customizations are needed.

That sounds good to me. It seems sufficiently different to justify a new ticket, so I'll close this ticket and PR and we can move further discussion over to #25146.

Thanks for the great feedback, everyone.

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