Iterate plugin results

[fschulze]

Currently we have the firstresult option. An interesting addition would be a way to iterate over the plugins. The big difference to the default of returning all results, is that the consumer can stop iterating at any point. This is useful for things where plugins might cause user interaction. One example is a password hook that tries to get a password from a keychain and if that doesn't work the next plugin is tried, which asks the user for the password in a prompt. With firstresult we don't get the fallback and with the current default of returning all results, the user would always be prompted, even if the password from the keychain would work.

[RonnyPfannschmidt]

at first glance this will be a massive pain and incompatible with hookwrappers

i believe we should have n actually discussion about the patterns of control that happen, perhaps take a look at stevedore as well

otherwise we will just repeat the stockpiling of minimal changes that made the pytest codebase a painful mess

[goodboy]

@fschulze if I understand this correctly you're looking for something like:

for result in pm.hook.myproject_get_password(arg1=1, arg2=2):
    if not result:  # skip to the next hookimpl
        continue
    else:
        success = unlock_with_pw(result)
        if success:
            break

Where each iteration lazily invokes each underlying hookimpl?

@RonnyPfannschmidt actually this wouldn't be incompatible with wrappers as long as we can remove the recursion non-sense from _MultiCall.execute() which is lingering because #23 hasn't been addressed. I already have a solution in my head.

[goodboy]

@fschulze I looked into this briefly and it seems to me in order to avoid transforming pluggy.callers._multicall() into a generator function (which has a ~4x slowdown per call if use like a function) we would need an almost identical alternative implementation.

We could try it as a trial feature that we don't have to commit to keeping but I'd want to hear what everyone else thinks.

[RonnyPfannschmidt]

@goodboy do you have that experiment ready? if yes i propose to just do a RFC pr and others taking a look, we should probably let that one trigger a futurewarning thats not hidden

[goodboy]

@RonnyPfannschmidt it's basically just rewriting that function to yield values in stead of appending them the list. The only real challenge is how to expose it.

I can draft up something if I get the time this weekend.

[nicoddemus]

I agree, if it is something simple we can discuss it over a PR.

The only real challenge is how to expose it.

IIUC, actually we would change the existing interface, no? Currently pm.hook.myproject_get_password(...) returns a list of results, we would change that to return an iterator instead.

I'm 👍 on the idea, Python itself change its methods which returned lists to iterator-like (dict.keys(), etc).

It will break the API, but now big deal because it should be simple to fix and we can coordinate that with pytest, tox and devpi.

[RonnyPfannschmidt]

well, actually - its not really a api break to intorduce it as aoption, but it would make hookwrapper incompatible for example

as such we need to take a close look on how to express this api because its effects are far from simple

[nicoddemus]

but it would make hookwrapper incompatible for example

Hmm can you explain why? After all even if the hook caller decides to stop the iteration, the "after" hooks can still be called I believe.

[goodboy]

After all even if the hook caller decides to stop the iteration, the "after" hooks can still be called I believe.

Yep, totally agree it's straightforward. ~with the only caveat (at least if we wanted to implement this using a generator) being that if the user wishes to kill further iterations they'll have to explicitly .send() a value to trigger teardown:~

iter_hooks = pm.ihook.myproject_get_password(arg1=1, arg2=2)
for result in iter_hooks:
    if not result:  # skip to the next hookimpl
        continue
    else:
        success = unlock_with_pw(result)
        if success:
            # don't call any more hooks and teardown
            iter_hooks.send("None")

Scratch that you can do it with a finally: block inside the generator and it works.

[RonnyPfannschmidt]

my gut feeling screams this will kill us with strange edge cases unless we correctly combine it with context management in some ways

[goodboy]

@RonnyPfannschmidt I could see that yeah. I like context managers 👍 - explicit is always better.

[nicoddemus]

Just breaking out of the loop should work, so I think this should be possible:

iter_hooks = pm.ihook.myproject_get_password(arg1=1, arg2=2)
for result in iter_hooks:
    if not result:  # skip to the next hookimpl
        continue
    else:
        success = unlock_with_pw(result)
        if success:
            break

Or more succinctly:

iter_hooks = pm.ihook.myproject_get_password(arg1=1, arg2=2)
for result in iter_hooks:
    success = unlock_with_pw(result)
    if success:
        break

And hook wrappers would still work, you can call pre/post if you use a try/finally while yielding the hook results. Here's a short demonstration:

def call_hooks():
    r = []
    try:
        hool_impls = range(5)
        for x in hool_impls:
            yield x
            r.append(x)
    finally:
        print('call post hookwrappers', r)

for i in call_hooks():
    print('hook', i)
    if i == 2:
        print('breaking')
        break

This prints:

hook 0
hook 1
hook 2
breaking
call post hookwrappers [0, 1]

[nicoddemus]

(I just noticed @tgoodlet pushed a PR, I will review it in the context of this implementation when I get the chance)

[RonnyPfannschmidt]

@fschulze @goodboy @nicoddemus

i have just been thinking, why not inverse the flow of control in the example

iter_hooks = pm.ihook.myproject_get_password(arg1=1, arg2=2)
for result in iter_hooks:
    if not result:  # skip to the next hookimpl
        continue
    else:
        success = unlock_with_pw(result)
        if success:
            break

is the wrong way around

class KeyringPlugin:
   def __init__(self, keyring):
      ...
   def myproject_unlock_identity(unlock_with_pw ,arg1, arg2)
       try:
           password = ...
       except (ValueError, LookupError):
         pass
       else:
           return unlock_with_pw(password)

# firstresult hook
success = pm.ihook.myproject_unlock_identity(unlock_with_pw=unlock_with_pw,arg1=1, arg2=2)

unless someone provides a example where that pattern cannot hope to work, i propose we drop the idea and keep pluggy simpler

[nicoddemus]

Good point. I'm OK with suggesting that alternative.

If we agree @RonnyPfannschmidt suggestion is reasonable, this would then not be a 1.0 release blocker, I assume.

[fschulze]

That way the KeyringPlugin needs to know about the password plugin.

I don't think this should block a 1.0 either way, but I still think if we find a good way to implement it, the feature would be worth adding. Do we have a low priority label we could set on this ticket?