Gonna add this in here as well as ticket #14512 I think using decorators to modify the way a CBV behaves is the wrong way to go about it, my reasoning is:
1) Decorators on functions make sense, since the only way to modify a function is to wrap it, so decorators provide a shortcut to do so. 2) There's already a standard way to add functionality to a class that has existed for a long time, is well tested, and as a bonus is something that people new to python but not new to programming will understand immediately. 3) Decorating a class is different to how adding any other kind of functionality to a CBV is done. 4) Customizability. Currently your only options to change the way one of the current decorators work is to rewrite it, unless the original author of the decorator thought of your use case, thought it was valid, and added a flag/option for it. - This can be alleviated by using Object based decorators instead of function based, but that becomes uglier too because then you end up having to first subclass the decorator, then decorate the class. 5) Conceptually all the decorator really is doing is mixing in one method anyways, it's just hiding this away and adding more code and complexity, and reducing customizability and readability for the sole purpose of being able to use an @ symbol. 6) Forwards compatibility. The function based generic views have been deprecated. While function based views will probably always be supported, personally I think CBV's are the way forward, and it makes sense to have them be first class citizens as regards to adding in things like requiring logins, with the function based views having the helper code. 7) Backwards compatibility. With a (much simpler) bit of wrapper code, the decorators can easily still be supported (login_required etc) and can internally use the classes and present the same interface as they used to. The major difference being, now they'll be able to subclass them and customize tiny bits of it as well. To just expand a little more in general on this @login_required class ProtectedView(TemplateView): template_name = "secret.html" vs class ProtectedView(LoginRequired, TemplateView): template_name = "secret.html" In the first, the "things" defining how this CBV behaves are in two locations, which makes it harder to read, additionally you end up with a function that wraps a function that wraps a function (to some degree of wrapping) to actually get all of this to work. Each layer of wrapping adds another layer of indirection, and adds another layer of confusion for someone attempting to read the code. I really dumbed down example would be https://gist.github.com/1220512 . Obviously that isn't working code, but it gives a general idea of what I mean. Obviously if that is the path that is chosen it will need to be cleaned up and made to actually work. Hopefully this email makes sense. I just hope that since CBV's are new we can use this as an opportunity to do things in a cleaner, consistent and more generic way instead of continuing the old way (that made sense for functions) for the sake of doing it the same way. tl;dr; Using Mixins to add in functionality to a CBV makes way more sense then using a decorator which is essentially going to be doing the same thing as a mixing would, it just makes finding what is going on harder, makes customizing the decorator harder and/or uglier, and goes against the way functionality is currently added to a CBV. On Thursday, September 15, 2011 at 4:44 PM, Jacob Kaplan-Moss wrote: > Hi folks -- > > I'd like to convert all the view decorators built into Django to be > "universal" -- so they'll work to decorate *any* view, whether a > function, method, or class. I believe I've figured out a technique for > this, but I'd like some feedback on the approach before I dive in too > deep. > > Right now view decorators (e.g. @login_required) only work on > functions. If you want to use a decorator on a method then you need to > "convert" the decorator using method_decorator(original_decorator). > You can't use view decorators on class-based views at all. This means > making a class-based view require login requires this awesomeness:: > > class MyView(View): > @method_decorator(login_required) > def dispatch(self, *args, **kwargs): > return super(MyView, self.dispatch(*args, **kwargs) > > This makes me sad. It's really counter-intuitive and relies on a > recognizing that functions and methods are different to even know to > look for method_decorator. > > #14512 proposes a adding another view-decorator-factory for decorating > class-based views, which would turn the above into:: > > @class_view_decorator(login_required) > class MyView(View): > ... > > This makes me less sad, but still sad. Factory functions. Ugh. > > I want @login_required to work for anything:: > > @login_required > class MyView(View): > ... > > class Something(object): > @login_required > def my_view(self, request): > ... > > @login_required > def my_view(request): > ... > > > Now, back in the day we somewhat had this: decorators tried to work > with both functions and methods. The technique turned out not to work > (see #12804) and was removed in [12399]. See > http://groups.google.com/group/django-developers/browse_thread/thread/3024b14a47f5404d > for the discussion. > > I believe, however, I've figured out a different technique to make > this work: don't try to detect bound versus unbound methods, but > instead look for the HttpRequest object. It'll either be args[0] if > the view's a function, or args[1] if the view's a method. This > technique won't work for any old decorator, but it *will* work (I > think) for any decorator *designed to be applied only to views*. > > I've written a proof-of-concept patch to @login_required (well, > @user_passes_test, actually): > > https://gist.github.com/1220375 > > The test suite passes with this, with one exception: > https://code.djangoproject.com/browser/django/trunk/tests/regressiontests/decorators/tests.py#L87. > I maintain that this test is broken and should be using RequestFactory > instead. > > Can I get some thoughts on this technique and some feedback on whether > it's OK to apply to every decorator built into Django? > > Jacob > > -- > You received this message because you are subscribed to the Google Groups > "Django developers" group. > To post to this group, send email to django-developers@googlegroups.com > (mailto:django-developers@googlegroups.com). > To unsubscribe from this group, send email to > django-developers+unsubscr...@googlegroups.com > (mailto:django-developers+unsubscr...@googlegroups.com). > For more options, visit this group at > http://groups.google.com/group/django-developers?hl=en. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
