Iterate plugin results about pluggy HOT 16 OPEN

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

from pluggy.

@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.

from pluggy.

@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.

from pluggy.

@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

from pluggy.

@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.

from pluggy.

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.

from pluggy.

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

from pluggy.

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.

from pluggy.

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.

from pluggy.

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

from pluggy.

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

from pluggy.

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]

from pluggy.

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

from pluggy.

@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

from pluggy.

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.

from pluggy.

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?

from pluggy.