On 21 October 2010 16:53, Luke Plant <l.plant...@cantab.net> wrote:
> 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.
To me, both versions aren't obvious and definitly not something I
would except from a decorator - to run only once. But I don't have any
better ideas myself, so I won't complain.
>
>> 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.
I thought for a moment there, that modifing the original is inline
with how function decorators work, but you're right. Do you consider
subclassing inside a decorated bad, or is it ok? With subclassing,
this is pretty much
the same as converting a decorator to a Mixin:
class MyView(Decorator(login_required), View):
pass
Of course there's an issuse with MRO and class identity.
>
> 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))
Agreed. 1 and 3 should be the same. 2 is actually broken. My original
example should be:
class MyView(View):
@classmethod
def get_decorators(cls):
return (login_required,) + super(cls).get_decorators()
That's a big -1 for this approach, 'cause it's error prone. If someone
forgets the super() call it just resets the whole decorator stack (at
least in my naive implementation).
>
> 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.
Consider:
@class_decorator(dec_x)
class A(View):
def dispatch():
#some code A
@class_decorator(dec_y)
class B(A)
def dispatch():
#some code B
What is the order of code execution:
* (dec_y, codeB, dec_x, codeA) or
* (dec_y, dec_x, codeB, codeA).
I think it's the first option - so the anwser would be "shortcut to
defining the intrinsic functionality". But we already have one
mechanism to create this kind of shortcuts - Mixins.
Another question is, how to handle duplicate decorators. If I subclass
from 2 views that already have login_required(), should the decorator
be applied twice or should decorators be unique and be ordered just
like the classes they are defined in? First can be impractical, second
can be very non-obvious. It's hard to choose.
--
Ł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 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.
