Re: [Django] #13879: method_decorator doesn't supports decorators with arguments
Django
-------------------------------------+-------------------------------------
Reporter: marinho | Owner: adurdin
Type: New | Status: assigned
feature | Component: Core (Other)
Milestone: 1.3 | Severity: Normal
Version: 1.2 | Keywords: sprintnov13,
Resolution: | decorators
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by adurdin):
* owner: nobody => adurdin
* status: new => assigned
* easy: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:6>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: New | Status: new
Milestone: 1.3 | Severity: Normal
Version: 1.2 | Keywords: sprintnov13,
Resolution: | decorators
Patch needs improvement: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by adurdin):
* status: assigned => new
* owner: adurdin => nobody
* has_patch: 0 => 1
Comment:
Implemented lukeplant's suggestion of `method_decorator_with_args`.
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: marinho | Owner: nobody
Type: New | Status: new
feature | Component: Core (Other)
Milestone: 1.3 | Severity: Normal
Version: 1.2 | Keywords: sprintnov13,
Resolution: | decorators
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
-------------------------------------+-------------------------------------
Changes (by jezdez):
* needs_better_patch: 0 => 1
Comment:
I'm not terribly happy with the `..with_args` suffix (same for
`decorator_from_middleware_with_args`). It feels weird to having to
remember to 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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: marinho | Owner: nobody
Type: New | Status: new
feature | Component: Core (Other)
Milestone: 1.3 | Severity: Normal
Version: 1.2 | Keywords: sprintnov13,
Resolution: | decorators
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 1 |
-------------------------------------+-------------------------------------
Changes (by jezdez):
* stage: Accepted => Design decision needed
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: marinho | Owner: nobody
Type: New | Status: new
feature | Component: Core (Other)
Milestone: 1.3 | Severity: Normal
Version: 1.2 | Keywords: sprintnov13,
Resolution: | decorators
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 1 |
-------------------------------------+-------------------------------------
Comment (by lukeplant):
@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
[http://code.djangoproject.com/browser/django/trunk/django/views/decorators/cache.py#L7
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:
{{{
#!python
@method_decorator(permission_required, 'spam.more_eggs',
another_arg=True)
def method(self, whatever):
}}}
is no improvement over this code:
{{{
#!python
@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: marinho | Owner: nobody
Component: Utilities | Version: 1.2
Severity: Normal | Resolution:
Keywords: sprintnov13, | Triage Stage: Design
decorators | decision needed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* component: Core (Other) => Utilities
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: marinho | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: 1.2
Severity: Normal | Resolution:
decorators | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* stage: Design decision needed => Accepted
Comment:
I grudgingly acknowledge the opportunity of adding
`method_decorator_with_args`, for consistency with
`decorator_from_middleware_with_args`.
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:13>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Component: Utilities | Version: 1.2
Severity: Normal | Resolution:
Keywords: sprintnov13, | Triage Stage: Accepted
decorators | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => susan
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: marinho | Owner: susan
Type: New feature | Status: assigned
Component: Utilities | Version: 1.2
Severity: Normal | Resolution:
Keywords: sprintnov13, | Triage Stage: Accepted
decorators | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by susan):
Applied the attached patch and all the tests pass.
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: marinho | Owner: susan
Type: New feature | Status: assigned
Component: Utilities | Version: 1.2
Severity: Normal | Resolution:
Keywords: sprintnov13, | Triage Stage: Accepted
decorators | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: douglas.russell@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: marinho | Owner:
Severity: Normal | Resolution:
Keywords: sprintnov13, | Triage Stage: Accepted
decorators | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => new
* owner: susan =>
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: marinho | Owner:
Type: New feature | Status: new
Component: Utilities | Version: 1.2
Severity: Normal | Resolution:
Keywords: sprintnov13, | Triage Stage: Accepted
decorators |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: toracle@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:18>
Django
-------------------------------------+-------------------------------------
Severity: Normal | Resolution:
Keywords: sprintnov13, | Triage Stage: Accepted
decorators |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => Wes P.
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Marinho Brandão | Owner: Wes P.
Type: New feature | Status: assigned
Component: Utilities | Version: 1.2
Severity: Normal | Resolution:
Keywords: sprintnov13, | Triage Stage: Accepted
decorators |
Has patch: 1 | Needs documentation: 0
-------------------------------------+-------------------------------------
Changes (by Wes P.):
Comment:
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.
https://github.com/django/django/pull/19333
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Marinho Brandão | Owner: Wes P.
Component: Utilities | Version: 1.2
Severity: Normal | Resolution: wontfix
decorators |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: => wontfix
* status: assigned => closed
Comment:
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 [https://github.com/orgs/django/projects/24 new feature tracker] and
reopen if there's clear agreement from the community this is wanted.
--
Ticket URL: <https://code.djangoproject.com/ticket/13879#comment:21>