Opened 14 years ago

Closed 11 years ago

#14512 closed New feature (wontfix)

Documentation & tools for decorating class-based-views.

Reported by: Łukasz Rekucki Owned by: Łukasz Rekucki
Component: Generic views Version: dev
Severity: Normal Keywords: class-based-views
Cc: me@…, Łukasz Rekucki, preston@…, Donald Stufft Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (6)

ticket14512_draft.diff (15.0 KB ) - added by Łukasz Rekucki 14 years ago.
Draft patch presenting 2 ways of decoration.
ticket14512.diff (13.8 KB ) - added by Łukasz Rekucki 14 years ago.
Cleaned up the patch a bit.
ticket14512.2.diff (7.5 KB ) - added by Roald de Vries 13 years ago.
patch based on release 1.3.1
ticket14512.3.diff (3.8 KB ) - added by Roald de Vries 13 years ago.
Improvement of ticket14512.2.diff
ticket14512.4.diff (8.5 KB ) - added by Roald de Vries 13 years ago.
Same patch as previous, but now complete
DecoratorMixin.py (726 bytes ) - added by tobia 13 years ago.
Alternative, simpler way to use old-style decorators with CBV

Download all attachments as: .zip

Change History (31)

comment:1 by Valentin Golev, 14 years ago

Cc: me@… added

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

comment:2 by Valentin Golev, 14 years ago

Needs documentation: set
Needs tests: set

comment:3 by Gabriel Hurley, 14 years ago

Needs documentation: unset
Needs tests: unset
Triage Stage: UnreviewedAccepted

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.

comment:4 by Łukasz Rekucki, 14 years ago

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.

by Łukasz Rekucki, 14 years ago

Attachment: ticket14512_draft.diff added

Draft patch presenting 2 ways of decoration.

comment:5 by Łukasz Rekucki, 14 years ago

Cc: Łukasz Rekucki added
Has patch: set

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.

by Łukasz Rekucki, 14 years ago

Attachment: ticket14512.diff added

Cleaned up the patch a bit.

comment:6 by Russell Keith-Magee, 14 years ago

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.

comment:7 by Alex Gaynor, 14 years ago

Resolution: fixed
Status: newclosed

Fixed by [14642].

comment:8 by Roald de Vries <roald@…>, 13 years ago

Easy pickings: unset
Resolution: fixed
Severity: Normal
Status: closedreopened
Type: Uncategorized
UI/UX: unset

comment:9 by Preston Holmes, 13 years ago

Cc: preston@… added

by Roald de Vries, 13 years ago

Attachment: ticket14512.2.diff added

patch based on release 1.3.1

comment:10 by Roald de Vries, 13 years ago

milestone: 1.31.4
Type: UncategorizedNew feature

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

comment:11 by Simon Charette, 13 years ago

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) 

by Roald de Vries, 13 years ago

Attachment: ticket14512.3.diff added

Improvement of ticket14512.2.diff

comment:12 by Roald de Vries, 13 years ago

  • 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

comment:13 by Preston Holmes, 13 years ago

Patch needs improvement: set

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

comment:14 by Preston Holmes, 13 years ago

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.

by Roald de Vries, 13 years ago

Attachment: ticket14512.4.diff added

Same patch as previous, but now complete

comment:15 by Roald de Vries, 13 years ago

Patch needs improvement: unset

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

comment:16 by Preston Holmes, 13 years ago

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.

Version 0, edited 13 years ago by Preston Holmes (next)

in reply to:  11 comment:17 by Donald Stufft, 13 years ago

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.

comment:18 by Donald Stufft, 13 years ago

Cc: Donald Stufft added

comment:19 by Donald Stufft, 13 years ago

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.

comment:20 by Jacob, 13 years ago

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.

comment:21 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:22 by Preston Holmes, 13 years ago

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

by tobia, 13 years ago

Attachment: DecoratorMixin.py added

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

comment:23 by daenney, 12 years ago

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)

comment:24 by Aymeric Augustin, 12 years ago

Status: reopenednew

comment:25 by Garry Polley, 11 years ago

Resolution: wontfix
Status: newclosed

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

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

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