On Thu, 2010-10-21 at 15:02 +0200, Łukasz Rekucki wrote:
> > Good catch! The attached patch should fix it, but it feels a bit icky,
> > due to the double application of the decorator. Let me know if it solves
> > this problem for you, or if you can think of a better way to fix it.
>
> I does fell icky. I haven't found a better way to solve this in
> general case, so I started thinking about a solution designed for CBV
> specifically.
method_decorator does need fixing anyway, and the fix isn't as bad as I
first thought. The existing implementation already suffered from the
following issue: when you decorate a top-level function, the decorator
runs at the time the module is imported. But when you use the same
decorator with method_decorator on a method in a top-level class, the
decorator actually runs every time that method is called - *not* when
the module is imported and the class is defined. With my change, the
decorator also runs once when the class is defined, in addition to every
time the method is called.
> Both are a variation on what you proposed earlier:
>
> 1) Add get_decorators() class method, that as is called by as_view(),
> to get a list of decorators
> to be applied on the closure.
>
> class MyView(View):
> @classmethod
> def get_decorators(cls):
> return (login_required,)
>
> This can be folded to a class decorator:
>
> def decorate_class(fdec):
> @wraps(fdec)
> def _decorator(cls):
> original_method = cls.get_decorators
> @wraps(original_method)
> def method_replacement(cls):
> return (fdec,) + original_method()
> cls.get_decorators = method_replacement
> return cls
> return _decorator
>
> @decorate_class(login_required):
> class MyView(View):
> pass
This one seems much better than the decorators class property I
suggested - no hacking with MRO. Encouraging the use of the class
decorator seems like the way to go. However, there are some points with
your implementation:
1) It mutates the original class which is not a good idea really. I
should be able to do this:
MyView1 = decorate_class(login_required)(MyView)
MyView2 = decorate_class(something_else)(MyView)
and not have login_required applied to MyView to MyView2
That means subclassing inside decorate_class.
2) We need to be careful about the order in which decorators are
applied. I would expect the following 3 chunks of code to be the
equivalent:
@decorate_class(dec1)
@decorate_class(dec2)
class MyView(View):
# ....
class MyView(View):
def get_decorators(self):
return [dec1, dec2]
class MyView(View):
@method_decorator(dec1)
@method_decorator(dec2)
def dispatch(self, request, *args, **kwargs):
#...
All three should effectively be doing something like:
view = dec1(dec2(view))
3) And there is a general design issue. With this get_decorators()
method, there is both flexibility and some surprising behaviour: we
could write something like this:
@decorate_class(dec1)
@decorate_class(dec2)
class MyView(View):
# ....
and someone else could write:
class MySubClass(MyView):
@classmethod
def get_decorators(self):
return []
and dec1 and dec2 would not be applied. I don't know if this is a good
idea or not. If you consider decorators to be part of the
configurability that class based views are designed to support, then it
does make sense. If you consider them as a shortcut to defining the
intrinsic functionality of a view, it doesn't.
Luke
--
If you can't answer a man's arguments, all is not lost; you can
still call him vile names. (Elbert Hubbard)
Luke Plant || http://lukeplant.me.uk/
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-develop...@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.
