On 15 September 2011 23:27, Donald Stufft <[email protected]> wrote: > 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 [email protected]. > To unsubscribe from this group, send email to > [email protected]. > 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 [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/django-developers?hl=en. >
-- Łukasz Rekucki -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
