Opened 11 years ago

Closed 8 years ago

Last modified 7 years ago

#20223 closed New feature (fixed)

Allow allow_lazy to be used as a decorator

Reported by: Alexey Boriskin Owned by: Iacopo Spalletti
Component: Utilities Version: dev
Severity: Normal Keywords:
Cc: bmispelon@…, github@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (9)

comment:1 by Baptiste Mispelon, 11 years ago

Cc: bmispelon@… added
Triage Stage: UnreviewedAccepted

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:

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

comment:2 by Luke Plant, 11 years ago

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.

comment:3 by Baptiste Mispelon, 11 years ago

Owner: changed from nobody to Baptiste Mispelon
Status: newassigned

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

comment:4 by Baptiste Mispelon, 11 years ago

I've prepared a pull request for this ticket that include fixes for 3 other ones: https://github.com/django/django/pull/1007

comment:5 by Alexey Boriskin, 9 years ago

I've updated bmispelon's patch so it merges cleanly as a pull request 4202:
https://github.com/django/django/pull/4202

comment:6 by Iacopo Spalletti, 8 years ago

Owner: changed from Baptiste Mispelon to Iacopo Spalletti

Assigning this to me to rebase and help get this merged

comment:7 by Iacopo Spalletti, 8 years ago

Cc: github@… added
Has patch: set

Rebased version of the above PRs opened at https://github.com/django/django/pull/5581

comment:8 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In d693074:

Fixed #20223 -- Added keep_lazy() as a replacement for allow_lazy().

Thanks to bmispelon and uruz for the initial patch.

comment:9 by Tim Graham <timograham@…>, 7 years ago

In 9d304b26:

Refs #20223 -- Removed deprecated django.utils.functional.allow_lazy().

Note: See TracTickets for help on using tickets.
Back to Top