Decorator syntax for reader pairs

[j4mie]

Often django-readers codebases have a lot of code that looks something like this:

prepare_upper_foo = qs.include_fields("foo")

def produce_upper_foo(instance):
    return instance.foo.upper()

upper_foo = prepare_upper_foo, produce_upper_foo

This is fine and conceptually straightforward, but it is quite verbose and creates three entries in the module namespace, only one of which is actually "public API": the upper_foo pair that would be used in a spec. However, the actual business logic is usually in the produce_<xxx> function, which means it usually makes sense to import just this function in your tests in order to unit test it.

I have been thinking that the common case might be nicely implemented with a decorator to "attach" the queryset function to the producer, something like this:

@prepare(qs.include_fields("foo"))
def upper_foo(instance):
    return instance.foo.upper()

A naive implementation for this decorator might be:

def prepare(queryset_function):
    def decorator(producer_function):
        return queryset_function, producer_function
    return decorator

However, the downside of this is that, once decorated, what was the upper_foo function in the module namespace would no longer be a callable function: it would be a tuple. While decorators can do this, it's a highly unusual thing to do and would be fairly surprising to someone reading the code. Also, in order to test the producer function in isolation, one would have to unpack the pair before calling its second item.

One way round this might be to attach the queryset function as an attribute to the producer, something like this:

def prepare(queryset_function):
    def decorator(producer_function):
        producer_function.queryset_function = queryset_function
        return producer_function
    return decorator

This would work, and it has some precedence in Django (the display decorator in the admin, for example), but this would require deep changes (both conceptual changes and code changes) to django-readers itself, to enable it to support either a two-tuple or a function-with-an-attribute in all the code that expects pairs to be two-tuples. This is not a refactoring I'd like to do, and I think it would add a lot of cognitive overhead to the library.

So to summarise, the two requirements are:

1. A decorator that behaves like a decorator, ie retains the fact that the decorated callable is still a callable.
2. Ensure that the decorated thing ends up as a pair (two-tuple) that can be understood by the rest of django-readers.

So.. what about if the decorator returned a thing that was both a two-tuple and a callable?

class CallablePair(tuple):
    def __call__(self, instance):
        return self[1](instance)

def prepare(queryset_function):
    def decorator(producer_function):
        return CallablePair((queryset_function, producer_function))
    return decorator

This means that with our code example from above:

@prepare(qs.include_fields("foo"))
def upper_foo(instance):
    return instance.foo.upper()

upper_foo would now be a CallablePair instance, which can be used directly in a spec (because it's a two-tuple), but also we could import the upper_foo "function" in a test and call it directly to test the business logic:

from some_package.readers import upper_foo

def test_upper_foo():
    instance = SomeModel(foo="hello")
    assert upper_foo(instance) == "HELLO"

Anyway, this is just an idea, and I'm very much looking for feedback on this. Is it too weird and surprising? Is it too clever? Is it even necessary?

Thanks :)

[pmg-certn]

Yes 🤔 . I like to inline it and return it immediately as a pair, reads more nicely to my eyes instead of polluting global namespace with things for no good reason:

upper_foo = (
    qs.include_fields("foo"),
    lambda instance: instance.foo.upper()
)

but that only works for small things like this, mostly due to restrictions in the language (lambdas arent real functions)

I don't really buy the argument However, the actual business logic is usually in the produce_<xxx> function, which means it usually makes sense to import just this function in your tests in order to unit test it for two reasons:

1. You shouldn't have your unit testing needs dictate your module interfaces, that's the tail wagging the dog.
2. In the unit test you just import the pair and test only the second element, simple enough to achieve if that's what you want

[charlie-certn]

I really like this.

I had been wondering for a while if we needed a simple/explicit way of defining a pair for the sake of re-enforcing the concepts but there was always that nagging feeling about why overcomplicate a tuple into something it didn't need to be long term for trade off with short term learning. Whilst this isn't that and not suggested for the same reasons it might have the side benefit with some of the initial learning if there's not three floating items to be defined.

I followed the reasoning above and it all seems like a very sensible conclusion. I do share similar thoughts to PMG on not pushing your consumer API to a worse state for the unit testing capabilities but I think you've come up with something that then works nicely solves that. One alternative (that you may have already considered an discarded) if you didn't want to make the whole thing callable is that you could expose each item of the pair for testing or use likeupper_foo.produce. However, you are then back in the realm of converting a function into a tuple via a decorator which is a bit odd, like you say.

[j4mie]

So.. a somewhat simpler pattern has been suggested to me, which requires no additional library support. With hindsight this is fairly obvious, and it's a well-established pattern I've used myself when parameterising pairs. The solution is.. just use the same pattern always, even if you don't need to parameterise.

Instead of:

prepare_upper_foo = qs.include_fields("foo")

def produce_upper_foo(instance):
    return instance.foo.upper()

upper_foo = prepare_upper_foo, produce_upper_foo

Instead, just put the extra names inside a closure:

def upper_foo():
    prepare = qs.include_fields("foo")

    def produce(instance):
        return instance.foo.upper()

    return prepare, produce

Then you can just use this in a spec by calling the upper_foo function.

[pmg-certn]

My issue with that is when you import something you don't know whether to use it like

{ 
   "thing": thing(),
}

or

{
   "thing": thing,
}

and a code reviewer is unlikely to spot the mistake if all a jumbled mixture. I think I've raised this before.

So IMO the function form should be make_thing() or get_thing() or something to clarify that (ie start with an imperative verb).

I think the underlying issue here is you're using a function because you want the scoping but you don't really need it to be a function. It's awkward.

[pmg-certn]

Javascript has the concept of IIFE -- immediately invoked function expression.

const foo = () => {
    // local scope variables
    // do things
    // return things
}();

I wonder if we could do something like that

def __make_upper_foo():
  prepare_upper_foo = qs.include_fields("foo")

  def produce_upper_foo(instance):
      return instance.foo.upper()

upper_foo = __make_upper_foo()

is about the best I can do