Opened 5 years ago

Closed 20 months ago

#14512 closed New feature (wontfix)

Documentation & tools for decorating class-based-views.

Reported by: lrekucki Owned by: lrekucki
Component: Generic views Version: master
Severity: Normal Keywords: class-based-views
Cc: me@…, lrekucki, preston@…, dstufft 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 lrekucki 5 years ago.
Draft patch presenting 2 ways of decoration.
ticket14512.diff (13.8 KB) - added by lrekucki 4 years ago.
Cleaned up the patch a bit.
ticket14512.2.diff (7.5 KB) - added by roalddevries 4 years ago.
patch based on release 1.3.1
ticket14512.3.diff (3.8 KB) - added by roalddevries 4 years ago.
Improvement of ticket14512.2.diff
ticket14512.4.diff (8.5 KB) - added by roalddevries 4 years ago.
Same patch as previous, but now complete
DecoratorMixin.py (726 bytes) - added by tobia 3 years ago.
Alternative, simpler way to use old-style decorators with CBV

Download all attachments as: .zip

Change History (31)

comment:1 Changed 5 years ago by valyagolev

  • Cc me@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 5 years ago by valyagolev

  • Needs documentation set
  • Needs tests set

comment:3 Changed 5 years ago by gabrielhurley

  • Needs documentation unset
  • Needs tests unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 5 years ago by lrekucki

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.

Changed 5 years ago by lrekucki

Draft patch presenting 2 ways of decoration.

comment:5 Changed 5 years ago by lrekucki

  • Cc lrekucki 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.

Changed 4 years ago by lrekucki

Cleaned up the patch a bit.

comment:6 Changed 4 years ago by russellm

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 Changed 4 years ago by Alex

  • Resolution set to fixed
  • Status changed from new to closed

Fixed by [14642].

comment:8 Changed 4 years ago by Roald de Vries <roald@…>

  • Easy pickings unset
  • Resolution fixed deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Uncategorized
  • UI/UX unset

comment:9 Changed 4 years ago by ptone

  • Cc preston@… added

Changed 4 years ago by roalddevries

patch based on release 1.3.1

comment:10 Changed 4 years ago by roalddevries

  • milestone changed from 1.3 to 1.4
  • Type changed from Uncategorized to New 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 follow-up: Changed 4 years ago by charettes

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) 

Changed 4 years ago by roalddevries

Improvement of ticket14512.2.diff

comment:12 Changed 4 years ago by roalddevries

  • 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 Changed 4 years ago by ptone

  • 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 Changed 4 years ago by ptone

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.

Changed 4 years ago by roalddevries

Same patch as previous, but now complete

comment:15 Changed 4 years ago by roalddevries

  • Patch needs improvement unset

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

comment:16 Changed 4 years ago by ptone

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.

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.

Last edited 4 years ago by ptone (previous) (diff)

comment:17 in reply to: ↑ 11 Changed 4 years ago by dstufft

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 Changed 4 years ago by dstufft

  • Cc dstufft added

comment:19 Changed 4 years ago by dstufft

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 Changed 4 years ago by 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.

comment:21 Changed 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:22 Changed 3 years ago by ptone

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

Changed 3 years ago by tobia

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

comment:23 Changed 2 years ago by 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)

comment:24 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:25 Changed 20 months ago by garrypolley

  • Resolution set to wontfix
  • Status changed from new to closed

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