[Django] #20223: Allow allow_lazy to be used as a decorator
Django
-----------------------------+--------------------
Reporter: void | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------
`allow_lazy` cannot be used as a decorator for now, at least not in
@-notation, because *resultclasses is required argument for this function.
Decorator with arguments are functions, which accept arguments, returning
function, which decorates source function. In allow_lazy signature
arguments and function are mixed.
So, proposal is to distinguish on type: if first argument of allow_lazy is
of type `type`, then treat it like a three-def-decorator, while allowing
current two-def-decorator behaviour if first argument is a function.
--
Ticket URL: <https://code.djangoproject.com/ticket/20223>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: void | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* cc: bmispelon@… (added)
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
I don't know if there is a rigorous definition of what a decorator is.
The glossary on python's documentation [1] says it's a "function that
returns a function".
In that sense, `allow_lazy` is indeed a decorator.
However, it's a bit unexpected that it's not compatible with the usual
@-syntax for decorators (because it takes arguments too).
It actually works with the @-syntax, but only if you don't pass it any
extra arguments. The problem is that these arguments are probably
required, though it's not very clear:
* The documentation [2] is unclear whether these are required or not.
* The docstring for `lazy` [3] is unambiguous: "You **need** to give
result classes or types" (emphasis mine)
* The code itself does not make sure of this and seems to when passed an
empty tuple.
I think the docstring is the one that's correct and that the extra
arguments are indeed required, as indicated by a ticket like #20222.
Personally, I'd like to see `allow_lazy` refactored into a proper two-step
decorator, with some checks to make sure we're actually passing the
required arguments:
{{{
#!python
def allow_lazy(*resultclasses):
"""
A decorator that allows a function to be called with one or more lazy
arguments. If none of the args are lazy, the function is evaluated
immediately, otherwise a __proxy__ is returned that will evaluate the
function when needed.
"""
if not resultclasses:
raise TypeError('You need at least one.')
def decorator(func):
@wraps(func)
def wrapper(*args, **kwargs):
for arg in list(args) + kwargs.values():
if isinstance(arg, Promise):
break
else:
return func(*args, **kwargs)
return lazy(func, *resultclasses)(*args, **kwargs)
return wrapper
return decorator
}}}
This breaks backwards-compatibility though, so we'd need to plan a
deprecation path.
It could be done quite simply by introducing a new `handle_lazy_args`
decorator (with a better name)
that would work with the @-syntax and have `allow_lazy` raise warnings.
I'm not sure I like the idea of type-checking the arguments like the OP
suggested,
though it might be another valid way to handle the deprecation path.
[1] http://docs.python.org/2/glossary.html
[2] https://docs.djangoproject.com/en/1.5/ref/utils/#module-
django.utils.functional
[3]
https://github.com/django/django/blob/master/django/utils/functional.py#L63-L66
--
Ticket URL: <https://code.djangoproject.com/ticket/20223#comment:1>
Django
Reporter: void | Owner: nobody
Type: New feature | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by lukeplant):
Please let's not have type checking to decide what type of thing to
return, because:
* isinstance is always a smell, and you can always find code that will
break it.
* it results in confusing code.
* whenever I have come across code that does this in the past, it has
caused problems, especially in higher-level situations (like decorating
the decorator), because the 'decorator' no longer functions predictably.
I'd be happy with a new function that acts as a decorator, like the
suggested `handle_lazy_args`.
--
Ticket URL: <https://code.djangoproject.com/ticket/20223#comment:2>
Django
Reporter: void | Owner: bmispelon
Type: New feature | Status: assigned
Component: Utilities | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* status: new => assigned
* owner: nobody => bmispelon
Comment:
It's kind of related to #20221 and #20222 so I'll try and fix those three
in one pull-request.
For anyone interested, I'll be working on this branch:
https://github.com/bmispelon/django/compare/allow-lazy-refactor
--
Ticket URL: <https://code.djangoproject.com/ticket/20223#comment:3>
Django
Reporter: void | Owner: bmispelon
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bmispelon):
I've prepared a pull request for this ticket that include fixes for 3
other ones: https://github.com/django/django/pull/1007
--
Ticket URL: <https://code.djangoproject.com/ticket/20223#comment:4>
Django
Reporter: void | Owner: bmispelon
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by void):
I've updated bmispelon's patch so it merges cleanly as a pull request
4202:
https://github.com/django/django/pull/4202
--
Ticket URL: <https://code.djangoproject.com/ticket/20223#comment:5>
Django
Reporter: void | Owner: yakky
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by yakky):
* owner: bmispelon => yakky
Comment:
Assigning this to me to rebase and help get this merged
--
Ticket URL: <https://code.djangoproject.com/ticket/20223#comment:6>
Django
Reporter: void | Owner: yakky
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by yakky):
* cc: github@… (added)
* has_patch: 0 => 1
Comment:
Rebased version of the above PRs opened at
https://github.com/django/django/pull/5581
--
Ticket URL: <https://code.djangoproject.com/ticket/20223#comment:7>
Django
Reporter: void | Owner: yakky
Component: Utilities | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d693074d431c50e4801dd6bf52525ce1436358f0" d693074]:
{{{
#!CommitTicketReference repository=""
revision="d693074d431c50e4801dd6bf52525ce1436358f0"
Fixed #20223 -- Added keep_lazy() as a replacement for allow_lazy().
Thanks to bmispelon and uruz for the initial patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20223#comment:8>
Django
Reporter: Alexey Boriskin | Owner: Iacopo
| Spalletti
Type: New feature | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"9d304b26cf2ce071a682bf68a29dee04d0e4cfdb" 9d304b26]:
{{{
#!CommitTicketReference repository=""
revision="9d304b26cf2ce071a682bf68a29dee04d0e4cfdb"
Refs #20223 -- Removed deprecated django.utils.functional.allow_lazy().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20223#comment:9>