Opened 15 years ago
Closed 4 months ago
#13879 closed New feature (wontfix)
method_decorator doesn't supports decorators with arguments
Reported by: | Marinho Brandão | Owned by: | Wes P. |
---|---|---|---|
Component: | Utilities | Version: | 1.2 |
Severity: | Normal | Keywords: | sprintnov13, decorators |
Cc: | Łukasz Rekucki, douglas.russell@…, toracle@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I tried use django.utils.decorators.method_decorator to support decorator permission_required and figured out it's not supporting decorators passing arguments like it does:
@permission_required(something_here)
Instead its code waits for you just pass a function as an only one argument, like this:
@login_required
I made my own version of it, that's below:
from functools import wraps, update_wrapper def method_decorator(decorator): """Converts a function decorator into a method decorator. This works properly for both: decorators with arguments and without them. The Django's version of this function just supports decorators with no arguments.""" # For simple decorators, like @login_required, without arguments def _dec(func): def _wrapper(self, *args, **kwargs): def bound_func(*args2, **kwargs2): return func(self, *args2, **kwargs2) return decorator(bound_func)(*args, **kwargs) return wraps(func)(_wrapper) # Called everytime def _args(*argsx, **kwargsx): # Detect a simple decorator and call _dec for it if len(argsx) == 1 and callable(argsx[0]) and not kwargsx: return _dec(argsx[0]) # Used for decorators with arguments, like @permission_required('something') def _dec2(func): def _wrapper(self, *args, **kwargs): def bound_func(*args2, **kwargs2): return func(self, *args2, **kwargs2) return decorator(*argsx, **kwargsx)(bound_func)(*args, **kwargs) return wraps(func)(_wrapper) return _dec2 update_wrapper(_args, decorator) # Change the name to aid debugging. _args.__name__ = 'method_decorator(%s)' % decorator.__name__ return _args
If necessary I can make a patch and attach to this ticket.
Attachments (1)
Change History (23)
comment:1 by , 15 years ago
Owner: | changed from | to
---|
comment:2 by , 15 years ago
Keywords: | sprintnov13 decorators added |
---|---|
milestone: | → 1.3 |
Owner: | changed from | to
Triage Stage: | Unreviewed → Design decision needed |
comment:3 by , 15 years ago
Cc: | added |
---|
IMHO, the goal here is to be able to write:
m_perm_required = method_decorator(permission_required) # do this once class A(View): @m_perm_required('spam.mores_spam') # use it later many times without repeating method_decorator def get(self, request, *args, **kwrgs): #...
The current implementation handles only decorators without arguments, so you can only write:
@method_decorator(permission_required('spam.more_eggs') def method(self, ...): #...
This is similar to diffrence between "decorator_from_middleware" and "decorator_from_middleware_with_args" and I think the right solution here is to provide a "method_decorator_with_args" function.
PS. The OP is wrong that his implementation handles all cases - there is no universal pattern in Python for transforming decorators with and without arguments if you allow the first argument to be a callable (a class for example). It's probably a matter of taste, so you can just disregard this part of comment :)
comment:4 by , 15 years ago
Triage Stage: | Design decision needed → Accepted |
---|
Technically, method_decorator
works as advertised. permission_required
is not actually a decorator. permission_required('spam.more_eggs')
is a decorator, and permission_required
is a decorator generator (it returns a decorator when you call it with some arguments).
But I agree that the goal set out lrekucki is a good one, and that a method_decorator_with_args
(or some better name) would be desirable, and that the OP's solution is not a good idea.
comment:5 by , 14 years ago
Component: | Uncategorized → Core (Other) |
---|---|
Severity: | → Normal |
Type: | → New feature |
comment:6 by , 14 years ago
Easy pickings: | unset |
---|---|
Owner: | changed from | to
Status: | new → assigned |
by , 14 years ago
Attachment: | ticket13879-r16259.diff added |
---|
Patch with fix and tests against trunk revision 16259
comment:7 by , 14 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | assigned → new |
Implemented lukeplant's suggestion of method_decorator_with_args
.
comment:8 by , 14 years ago
Patch needs improvement: | set |
---|
I'm not terribly happy with the ..with_args
suffix (same for decorator_from_middleware_with_args
). It feels weird to having to remember two different function names that do basically the same. Especially for decorators that take arguments optionally I'd appreciate if the method_decorator
function would simply allow me to pass the arguments for the decorator optionally, too. E.g.::
@method_decorator(permission_required, 'spam.more_eggs', another_arg=True) def method(self, whatever): # bla
comment:9 by , 14 years ago
Triage Stage: | Accepted → Design decision needed |
---|
After talking to adurdin on IRC more about it, I realized that this ticket shows why the "generate a method decorator" pattern is aweful. Marking as DDN.
comment:10 by , 14 years ago
@jezdez: My experience with decorators tells me that confusing decorators and decorator factories is always a bad idea, despite the temptation of convenience. If you look at the cache_page decorator, you'll find probably some of the nastiest code in Django, and it was caused by this confusion. You always end up with isinstance checks and horrible little edge cases, and it makes it hard to reason about the code, since it can accept/return fundamentally different types of things.
However, in this case, this code:
@method_decorator(permission_required, 'spam.more_eggs', another_arg=True) def method(self, whatever):
is no improvement over this code:
@method_decorator(permission_required('spam.more_eggs', another_arg=True)) def method(self, whatever):
in terms of the original motivation of this ticket, and it violates TOOWTDI.
comment:12 by , 12 years ago
Component: | Core (Other) → Utilities |
---|
comment:13 by , 12 years ago
Triage Stage: | Design decision needed → Accepted |
---|
I grudgingly acknowledge the opportunity of adding method_decorator_with_args
, for consistency with decorator_from_middleware_with_args
.
comment:14 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:16 by , 12 years ago
Cc: | added |
---|
comment:17 by , 12 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:18 by , 10 years ago
Cc: | added |
---|
comment:19 by , 5 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:20 by , 5 months ago
Patch needs improvement: | unset |
---|
After reviewing the ticket a few times, I also grudgingly agree that method_decorator_with_args
could be an added feature. I also think that it depends a lot on the values of the current team making the decision to include a new features or not. If consistency of functions and features is really important than yes, include the function. If reducing code and complexity is more important, than I would say set this ticket to "design decision needed" and then close it without accepting the pull request.
The feature has not come up again in a long time so it probably isn't that important. The feature doesn't add anything of real value to Django. The function method_decorator
can do the work and the tests are designed to make sure that arguments can be passed in when needed. Keeping method_decorator_with_args
adds more code that needs to be maintained.
The pull request includes the original patch provided by Susan Tan. I copied and modified all the tests for method_decorator
including the async tests. Some tests were not repeated, namely the tests for passing in a tuple of decorator factories since that is not part of the feature's design. I added documentation for the feature.
comment:21 by , 4 months ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
Thank you Wes for picking this up and bringing this ticket to the surface
I have consulted the steering council who have found comment:10 compelling, roughly that “it’s no improvement and violates one way to do it”.
That "grudgingly" has come up a few times in the ticket is a not a good sign.
Therefore, we will wontfix
the ticket. With the current discussion, there doesn't appear to be a strong agreement that this feature is wanted. If anyone wants to advocate for this, they are welcome to propose this in the new feature tracker and reopen if there's clear agreement from the community this is wanted.
I'm not sure about the goal of this request, so I'll leave the design decision to the core devs.