Opened 2 years ago

Last modified 5 months ago

#20223 assigned New feature

Allow allow_lazy to be used as a decorator

Reported by: void Owned by: bmispelon
Component: Utilities Version: master
Severity: Normal Keywords:
Cc: bmispelon@… Triage Stage: Accepted
Has patch: no 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 (5)

comment:1 Changed 2 years ago by bmispelon

  • Cc bmispelon@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 2 years ago 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.

comment:3 Changed 2 years ago by bmispelon

  • Owner changed from nobody to bmispelon
  • Status changed from new to assigned

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 Changed 2 years ago 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

comment:5 Changed 5 months ago by void

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

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