Code

Opened 15 months ago

Last modified 15 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.

Attachments (0)

Change History (4)

comment:1 Changed 15 months 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 15 months 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 15 months 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 15 months 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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from bmispelon to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.