Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Firstresult madness #72

Merged
merged 4 commits into from
Aug 30, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions pluggy/callers.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ def execute(self):
except BaseException:
excinfo = sys.exc_info()
finally:
outcome = _Result(results, excinfo)
if firstresult: # first result hooks return a single value
outcome = _Result(results[0] if results else None, excinfo)
else:
outcome = _Result(results, excinfo)

# run all wrapper post-yield blocks
for gen in reversed(teardowns):
Expand All @@ -106,10 +109,6 @@ def execute(self):
except StopIteration:
pass

if firstresult:
result = outcome.get_result()
return result[0] if result else None

return outcome.get_result()

def __repr__(self):
Expand Down
43 changes: 43 additions & 0 deletions testing/test_hookrelay.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,56 @@ class Plugin3(object):
def hello(self, arg):
return None

class Plugin4(object):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should add a new test which also changes the outcome result to make sure?

I plan to test this branch in pytest later today as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicoddemus yes good call.
I'll add a force result test.

@hookimpl(hookwrapper=True)
def hello(self, arg):
assert arg == 3
outcome = yield
assert outcome.get_result() == 2

pm.register(Plugin1()) # discarded - not the last registered plugin
pm.register(Plugin2()) # used as result
pm.register(Plugin3()) # None result is ignored
pm.register(Plugin4()) # hookwrapper should get same non-list result
res = pm.hook.hello(arg=3)
assert res == 2


def test_firstresult_force_result(pm):
"""Verify forcing a result in a wrapper.
"""
class Api(object):
@hookspec(firstresult=True)
def hello(self, arg):
"api hook 1"

pm.add_hookspecs(Api)

class Plugin1(object):
@hookimpl
def hello(self, arg):
return arg + 1

class Plugin2(object):
@hookimpl(hookwrapper=True)
def hello(self, arg):
assert arg == 3
outcome = yield
assert outcome.get_result() == 4
outcome.force_result(0)

class Plugin3(object):
@hookimpl
def hello(self, arg):
return None

pm.register(Plugin1())
pm.register(Plugin2()) # wrapper
pm.register(Plugin3()) # ignored since returns None
res = pm.hook.hello(arg=3)
assert res == 0 # this result is forced and not a list


def test_firstresult_returns_none(pm):
"""If None results are returned by underlying implementations ensure
the multi-call loop returns a None value.
Expand Down