_Result is unnecessary?

[bluetech]

In #257, @maxnikulin brought up an idea which seems very natural but hasn't occurred to me beforehand. I think it will at least be interesting to discuss it on its own.

Background

_Result is used in hookwrappers as follows (using imaginary type annotations for clarity):

from typing import Generator, TypeVar

T = TypeVar("T")
E = TypeVar("E")

@hookimpl(hookwrapper=True)
def my_hook() -> Generator[None, _Result[T, E], None]:
    # ... Some setup code ...

    result = yield

    try:
        value = result.get_result()
    except Exception as exc:
        # ... Use exc ...
    else:
        # ... Use value ...

    # ... Some teardown code ...

The result has three use cases:

1. Fetch the return value[s] of the hook, if it didn't raise.
2. Fetch the exception of the hook, if it did raise.
3. Override (force) the return value/exception to some other return value.

Idea

Python coroutines already provide a way to distinguish between a successful yield (-> coroutine.send(value)) and a throwing yield (-> coroutine.throw(exc)). So in the above, the _Result indirection could have been dispensed with:

@hookimpl(imaginary_hookwrapper=True)
def my_hook() -> Generator[None, T, None]:
    # ... Some setup code ...

    try:
        value = yield
    except Exception as exc:
        # ... Use exc ...
    else:
        # ... Use value ...

    # ... Some teardown code ...

This handles use cases 1 & 2. Regarding use case 3 (forcing a result), use the coroutine return value:

@hookimpl(imaginary_hookwrapper=True)
def my_hook() -> Generator[None, T, T]:
    # ... Some setup code ...

    try:
        value = yield
    except Exception as exc:
        # ... Use exc ...
    else:
        # ... Use value ...

    # ... Some teardown code ...

    return new_value

[RonnyPfannschmidt]

can we call the keyword argument result_processor, mutually exclusive with hookwrapper

an additional win is that we can now use raise new from old nicely

[oremanj]

It would be possible to handle this backwards-compatibly by encouraging hookwrappers to say value = (yield).get_result(). pluggy would still construct a _Result object for backcompat reasons, but it would immediately get unwrapped by newer hooks.

Then let the return value of the hookwrapper, or the exception it raises, be used to determine the result for the next layer up. Old code that uses force_result explicitly can still be supported.

[RonnyPfannschmidt]

@oremanj i dont quite follow, the new hook wrapping mechanism would not use result objects, the old hook wrapping mechanism would not get the new features

[oremanj]

If you have two mechanisms, you don't need my suggestion. If you want to stick with only one mechanism, my suggestion is a way to make it more pleasant to code against.

[maxnikulin]

Do not take too serious the following idea.

Even hookwrapper argument could be dropped. If a hook is a generator than it is a wrapper. But with such approach reading of the code requires more attention.

More seriously. Mutually exclusive keyword argument could be enforced by interpreter. It would be a source of confusion that both arguments could not be used simultaneously. Maybe other value of hookwrapper would be better (but likely still verbose enough).

@hookimpl(hookwrapper=pluggy.no_boxing)

[RonnyPfannschmidt]

i see where you want to go with that idea

in case we want to sort out the different hook control mechanisms, we might want to decompose controll types and ordering types from the current invocation,

as the arguments to hookimpl are bascially just a bag of flags, its entirely reasonable to have mutually exclusive parts, however i beleive a enerally better spelling is not done by special values/enums, but by distinct markers for the different controll flows

for example hookimpl could deprecate hookwrapper as argument alltogether,
and the next gen wrapping would be called something like hookcontroller (good names to be figured later)

as for hook ordering, i would like to see some better control over order relative to other plugins

as for invocation patterns,

it might be cool to have

@hook.spec
def my_hook(...): ...

@hook.control
def my_hook(...):
   try:
      yield
   finally:
      ...

@hook.impl
def my_hook(...):
   if ...:
      return ...

[bluetech]

Even hookwrapper argument could be dropped. If a hook is a generator than it is a wrapper.

This is another obvious-in-retrospect idea. We should summon you more :)

I think it's an interesting avenue to explore, in combination with dropping/deprecating _Result. When I get a chance, I'll try to approach it like this:

1. Remove hookwrapper and _Result (to start with).
2. Allow the hookimpl function to be a generator function (detected with inspect.isgeneratorfunction()).
3. If the function is a generator function, it has hookwrapper-like semantics, but as described in this issue, without _Result.
4. Add back hookwrapper/_Result support, for backward compatibility.
5. Favor the new style in the documentation.
6. Eventually consider deprecating hookwrapper (very disruptive so not for a few years at least).

Hopefully an actual implementation wouldn't poke any holes in this idea...

---

But with such approach reading of the code requires more attention.

I assume you're referring to reading the hookimpl code, that if we drop the explicit hookwrapper=True, it will be less obvious in a glance whether whether it is a wrapper or a regular impl.

I agree the explicit indication is nice, actually I think Python would have been better if it required something like gen def foo() for generator functions, like async def (and JavaScript function* syntax), but they didn't...

However:

1. This is a choice the Python devs made.
2. If type annotations are used, it will be typed as Generator[...], which serves as an indicator.

But if we want the explicitness anyway, we can add a new mutually exclusive boolean (my preference would be just wrapper=True).

[maxnikulin]

I suggested the check if some hook is a generator expecting that such proposal would be considered and conciosly declined.

I admit the argument concerning language design decision. Though I am not brave enough to prefer brevity here. Tests are special kind of code. Most of people do not like writing them or could not spend enough time for them, so quality of the code is lower than in the main part of a project. It is applicable even to hooks that may be a part of local plugins. As the number of tests grows at some certain amount the heap of tests become hardly manageable. Explicitly expressed intentions (even if language allows to omit some details) become invaluable during debugging of obscure failures.

"[hook]wrapper" is a hint related to the purpose, Generator type annotation (or gen def) describes implementation details. The former variant is more friendly to people who rarely
touche tests or testing framework plugins but sometimes have to deal with test failures.

Please, consider validation at startup phase in addition to errors during attempts to run tests, while adding mutually exclusive options (keyword arguments or old style keyword argument vs. implicit detection). In the ideal case, init time errors should be collected and inconsistencies in all hooks should be reported in a single run without execution of any tests. Such approach should be more informative in comparison to immediate exit on first error in the case of tests running by a CI system. It is convenient but not so important for interactive sessions.

Do you have an idea which keyword could be used when it would be decided to make "wrapper" obsolete? Short expressive terms make next choice harder.

[RonnyPfannschmidt]

i think this is similar to how we first had yield_fixture in pytest as separate tool and later merged the behaviour back into fixture