Documentation & tools for decorating class-based-views.

[Łukasz Rekucki]

Ticket for changes discussed at: ​http://groups.google.com/group/django-developers/browse_frm/thread/f4bad32127776177

• Current documentation of CBV should contain a section about decorating, describing how to use method_decorator() with existing decorators and CBV.
• Add a similar function to create a class decorator. (Maybe a decorator_to_mixin() too?)

I'll post a patch soon.

[Valentin Golev]

I thought we should have discussed the "right" way to implement it in mailing list before creating a ticket.

[Gabriel Hurley]

The ticket is fine so that we don't lose track of it. The description has a link to the thread in case someone stumbles upon it without knowing what decisions were/will be made on the list. But any patch should wait until "the right way" is agreed upon.

[Łukasz Rekucki]

Sorry if this came out badly. It wasn't my intention to try to force anything, but rather provide a draft patch to illustrate the discussed solutions.

[Łukasz Rekucki]

Draft patch presenting 2 ways of decoration.

[Łukasz Rekucki]

The attached patch is also available as a github branch: ​http://github.com/lqc/django/commits/cbvdecoration_ticket14512

I added some documentation, but it needs some work after we agree on the preferred method of decoration.

[Łukasz Rekucki]

Cleaned up the patch a bit.

[Russell Keith-Magee]

Hrm... I'm not wild about this implementation. subclass=False is a messy implementation detail that is hard to explain -- even the provided documentation does a Jedi Mind Track "don't do that"....

After some discussions on IRC, it seems like there may not be a clean way to handle this. I'm open to suggestions, but if the option is between a messy and brittle implementation of class decorators, and a mildly awkward but fundamentally predictable method decorator based solution, I'd rather go with the latter.

The docs in this patch are a good starting point for explaining how to do decoration in other ways; unless someone can propose a good way to implement class decorators, I'll close this ticket in favor of just documenting the alternatives.

[Alex Gaynor]

Fixed by [14642].

[Roald de Vries]

patch based on release 1.3.1

[Roald de Vries]

I attached a patch that does the job. It uses a copy of the original class instead of a subclass, which resolves the infinite recursion. The patch is made wrt django 1.3.1 (not trunk).

[Simon Charette]

I think the python 2.6 decorator syntax example you provide (in docs/topics/class-based-views.txt) decorates the class twice:

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

[Roald de Vries]

Improvement of ticket14512.2.diff

[Roald de Vries]

• corrected the documentation
• made the tests more comprehensible by using global namespace for cbv's that are used in the tests
• improved the patch in a few ways:
  • the as_view classonlymethod of the decorated class can now take parameters
  • the as_view doesn't rely on default function parameters anymore for correct resolution of the decorator and the original as_view
  • prevented super(SomeView, self) from raising a TypeError by basing the new as_view not on the original classonlymethod but on its underlying function

[Preston Holmes]

your latest patch only has the tests - it is not a complete patch, I'm pretty sure patches should not be compound patches here.

[Preston Holmes]

Looking at your latest patch again, looks like you simply generated the patch from inside the django/tests dir

Can you attach a corrected patch - I'm happy to help keep moving this forward, I think it is a big improvement.

[Roald de Vries]

Same patch as previous, but now complete

[Roald de Vries]

You guys are right (SVN inexperience), I corrected the patch.

[Preston Holmes]

In an effort to keep momentum on this issue, I was highlighting this approach in #django-dev

There was the desire to try to keep a simple unified approach to function-based and class-based views decoration

Accepting the assumption that you are only talking about decorating views - the following alternative approach was suggested by Jacob: ​https://gist.github.com/1219822

This removes the need for the double call ()() syntax in Python < 2.6

There were some subsequent comments that this would perhaps better live outside contrib.auth as some sort of general view utility.

​https://gist.github.com/1219822

Would be good to see brief pro/con consideration of these two approaches - keeping as accepted, but if this stalls, will need to unfortunately move to DDN, at which point I will try to summarize for list.

[Donald Stufft]

I for one think this is the wrong way of allowing someone to add something like requiring login to a class based view.

For one

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

Is ugly and janky. requiring a @view_decorator makes CBV's uglier and a second class citizen decorator wise because of legacy code. Using a decorator I would also argue is the wrong approach. There is already a well tested, backwards compatible (with regards to python), and language agnostic method of changing the behavior of a classes methods.

class ProtectedView(LoginRequired, TemplateView): 
    template_name = 'secret.html'

You can make a Mixin backwards compatible with function based views easily enough, and this puts all the information about where the functionality of a particular view is coming from (in the definition of the class, where it belongs in my opinion). Which is all you are really doing with your view_decorator approach, you are just hiding the fact you are mixing in functionality by using a decorator and dynamically subclassing and mixing in.

Additionally using a Mixin makes it easy to break the (now) decorators into multiple methods and allow the end user to easily modify a portion of it to fit their view. Currently with function based decorators this isn't possible, it's all or nothing. You can solve this problem by making class based decorators (which is sort of a Mixin anyways), but then you are adding even more code just so you can add a mixin via @ instead of the normal method of mixing in. An example use case of this is the guy who recently on the mailing list wanted login_required to check is_active. Currently his only option is to rewrite the login_required decorator.

[Donald Stufft]

ptone asked me to include an example:

This example is obviously not in a state to be included directly, but this is basically what I mean: ​https://gist.github.com/1220199

It's simple, doesn't require extra helper code to make it work, and anyone who can grok CBV's can get how that works without requiring deciphering multiple layers of decorator magic.

[Jacob]

Other committers: please hold off on committing this for now: I think I've figured out a way to make decorators Just Work™ for classes and for functions, obviating the need for view_decorator, mixins, or anything. I'd like to see if that technique is viable before introducing any of these workarounds. See ​http://groups.google.com/group/django-developers/browse_thread/thread/8eb7db7a70896185 for the discussion.

[Jacob]

Milestone 1.4 deleted

[Preston Holmes]

There was a fair amount of discussion around this issue in this thread:

​http://groups.google.com/group/django-developers/browse_thread/thread/8eb7db7a70896185

The approach of mixins as initially outlined in comment 19 and further flushed out by dstufft and myself below:

​https://github.com/dstufft/django/compare/master...mixin-decorators

I still consider to be the best approach

[tobia]

Alternative, simpler way to use old-style decorators with CBV

[daenney]

Anything still happening on this front?

There's a few mixin's laying around on djangosnippets and in third-party packages such as django-guardian and django-braces that usually do something like this:

class SomeDecoratorMixin(object):
    @method_decorator(some_decorator)
    def dispatch(self, *args, **kwargs):
        return super(SomeDecoratorMixin, self).dispatch(*args, **kwargs)

[Garry Polley]

Talked to Jacob, the solution here is to bring mixins into Django directly.

Since mixins are not decorators, this ticket is on longer valid.