WIP: split up and reshuffle hook calling - #2
Open
RonnyPfannschmidt wants to merge 1 commit into
Open
Conversation
Reviewer's GuideThis PR reshapes hook invocation by splitting hook implementations into separate wrapper and non-wrapper lists, refactors option normalization and result handling, and streamlines internal HookImpl and caller logic for clearer flows and better error reporting. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @RonnyPfannschmidt - I've reviewed your changes - here's some feedback:
- normalize_hookimpl_opts now returns a new dict instead of mutating in‐place—verify all code consuming hookimpl_opts uses the returned mapping and no in‐place updates or identity checks remain.
- Double-check that splitting into wrappers vs non-wrappers preserves the original hook invocation order (including tryfirst/trylast and nested wrappers) against existing behavior.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| hookwrappers.append(f) | ||
| else: | ||
| hookfuncs.append(f) | ||
| return caller("foo", hookwrappers, hookfuncs, kwargs, firstresult) |
There was a problem hiding this comment.
suggestion (testing): Consider adding targeted tests for new wrapper/non-wrapper call mechanics.
Add unit tests covering the new paths in _multicall and HookImpl.__call__:
- Execution order: wrapper setup (tryfirst/trylast), non-wrapper execution (tryfirst/trylast), then wrapper teardown in reverse.
- firstresult=True: wrappers still run setup and teardown even if a non-wrapper returns early.
- Calls with only wrappers.
- Wrapper exceptions both before and after the yield.
- Historic wrapper hooks exercising
_maybe_apply_history.
Suggested implementation:
return caller("foo", hookwrappers, hookfuncs, kwargs, firstresult)
# --------------------------------------------------------------------
# New unit tests for `_multicall` and `HookImpl.__call__`
# --------------------------------------------------------------------
import pytest
# adjust these imports to your actual module locations
from _pytest.config import _multicall, HookImpl
def test_wrapper_and_hook_execution_order():
events = []
@pytest.hookimpl(tryfirst=True, hookwrapper=True)
def wrap1():
events.append("setup_wrap1")
yield
events.append("teardown_wrap1")
@pytest.hookimpl(trylast=True, hookwrapper=True)
def wrap2():
events.append("setup_wrap2")
yield
events.append("teardown_wrap2")
@pytest.hookimpl(tryfirst=True)
def hook1():
events.append("hook1")
@pytest.hookimpl(trylast=True)
def hook2():
events.append("hook2")
# run with both wrappers and hooks
_multicall("foo", [wrap1, wrap2], [hook1, hook2], {}, firstresult=False)
assert events == [
"setup_wrap1",
"setup_wrap2",
"hook1",
"hook2",
"teardown_wrap2",
"teardown_wrap1",
]
def test_firstresult_true_wrappers_run():
events = []
@pytest.hookimpl(hookwrapper=True)
def wrap():
events.append("setup")
yield
events.append("teardown")
@pytest.hookimpl(tryfirst=True)
def early():
events.append("early")
return "result"
@pytest.hookimpl()
def never():
events.append("never")
result = _multicall("foo", [wrap], [early, never], {}, firstresult=True)
assert result == "result"
# even though 'never' never runs, setup and teardown do
assert events == ["setup", "early", "teardown"]
def test_only_wrappers():
events = []
@pytest.hookimpl(tryfirst=True, hookwrapper=True)
def w():
events.append("setup")
yield
events.append("teardown")
# no normal hook functions
_multicall("foo", [w], [], {}, firstresult=False)
assert events == ["setup", "teardown"]
def test_wrapper_exception_before_and_after():
@pytest.hookimpl(hookwrapper=True)
def bad_before():
raise RuntimeError("before yield")
yield # unreachable
with pytest.raises(RuntimeError):
_multicall("foo", [bad_before], [], {}, firstresult=False)
@pytest.hookimpl(hookwrapper=True)
def bad_after():
yield
raise RuntimeError("after yield")
with pytest.raises(RuntimeError):
_multicall("foo", [bad_after], [], {}, firstresult=False)
def test_historic_wrapper_hooks():
events = []
@pytest.hookimpl(hookwrapper=True)
def wrap_hist():
events.append("hsetup")
yield
events.append("hteardown")
# simulate that wrap_hist was used in a previous call
wrap_impl = HookImpl(None, "foo", wrap_hist, wrap_hist)
wrap_impl._was_called = True
# now call with only historic wrapper; it must still run
_multicall("foo", [wrap_impl], [], {}, firstresult=False)
assert events == ["hsetup", "hteardown"]- Ensure you have pytest installed and the correct import path for
_multicallandHookImplin your codebase. - If
_multicalllives under a different module (e.g.pytest._multicall), adjust thefrom … import _multicall, HookImplline accordingly. - Make sure the file ends with a newline after the appended tests.
|
|
||
| class HookCallError(Exception): | ||
| """Hook was called wrongly.""" | ||
| @final |
There was a problem hiding this comment.
issue (complexity): Consider replacing the manual class implementation with a dataclass using slots to simplify initialization and attribute management.
You’ve added a lot of boilerplate around storage and init. You can collapse most of it into a single `@dataclass(slots=True)` (Py-3.10+) and drop your manual `__slots__`, `__init__`, and the trivial `exception` property. For example:
```python
from dataclasses import dataclass, field
from types import TracebackType
from typing import Generic, Optional, TypeVar, Callable, final
R = TypeVar("R")
@final
@dataclass(slots=True)
class Result(Generic[R]):
result: Optional[R] = None
exception: Optional[BaseException] = None
_tb: Optional[TracebackType] = field(default=None, repr=False)
def __post_init__(self):
# stash the original traceback if we were given an exception
if self.exception and self._tb is None:
self._tb = self.exception.__traceback__
@property
def excinfo(self) -> tuple[type[BaseException], BaseException, Optional[TracebackType]] | None:
if not self.exception:
return None
return (type(self.exception), self.exception, self._tb)
@classmethod
def from_call(cls, func: Callable[[], R]) -> Result[R]:
__tracebackhide__ = True
try:
return cls(result=func())
except BaseException as e:
return cls(exception=e)
def force_result(self, result: R) -> None:
self.result = result
self.exception = None
self._tb = None
def force_exception(self, exc: BaseException) -> None:
self.result = None
self.exception = exc
self._tb = exc.__traceback__
def get_result(self) -> R:
__tracebackhide__ = True
if self.exception is None:
return self.result # type: ignore
raise self.exception.with_traceback(self._tb)Benefits:
- removes manual
__init__+__slots__ - drops the tiny
@property def exceptionin favor of a public field - keeps all existing APIs (
from_call,force_*,get_result,excinfo) - type‐safe and just ~30 lines of logic instead of ~60
Owner
Author
There was a problem hiding this comment.
dataclasses must be avoided in this library for its import and init cost
There was a problem hiding this comment.
Totally fair—if we’re shying away from dataclasses for import/init costs, we can stick with a handwritten class but still trim the boilerplate. Here’s one pattern that keeps __slots__ for memory savings, collapses the __init__ logic into one place, and drops the trivial exception property in favor of a public attribute:
from types import TracebackType
from typing import Generic, Optional, TypeVar, Tuple, Union, final
R = TypeVar("R")
@final
class Result(Generic[R]):
__slots__ = ("result", "exception", "_tb")
def __init__(
self,
result: Optional[R] = None,
exception: Optional[BaseException] = None,
tb: Optional[TracebackType] = None,
) -> None:
self.result = result
self.exception = exception
# if we got an exception but no tb, grab it
self._tb = tb if tb is not None or exception is None else exception.__traceback__
@property
def excinfo(self) -> Optional[Tuple[type[BaseException], BaseException, Optional[TracebackType]]]:
if self.exception is None:
return None
return (type(self.exception), self.exception, self._tb)
@classmethod
def from_call(cls, func: Callable[[], R]) -> "Result[R]":
__tracebackhide__ = True
try:
return cls(result=func())
except BaseException as e:
return cls(exception=e)
def force_result(self, result: R) -> None:
self.result = result
self.exception = None
self._tb = None
def force_exception(self, exc: BaseException) -> None:
self.result = None
self.exception = exc
self._tb = exc.__traceback__
def get_result(self) -> R:
__tracebackhide__ = True
if self.exception is None:
return self.result # type: ignore
raise self.exception.with_traceback(self._tb)Advantages versus the pre-rewrite version:
• One __init__ instead of separate slot declaration + init boilerplate
• Still uses __slots__ for minimal memory overhead
• No extra @property exception wrapper—just a public attr
• Clear TB handling in one place
If that still feels too verbose, we can leave it as you currently have it—the semantics are sound. Let me know what you think!
Comment on lines
+39
to
+41
| for method in hooks: | ||
| f = HookImpl(None, "<temp>", method, method.example_impl) | ||
| hook_impls.append(f) | ||
| nonwrappers.append(f) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Comment on lines
+42
to
+44
| for method in wrappers: | ||
| f = HookImpl(None, "<temp>", method, method.example_impl) | ||
| wrapping.append(f) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Comment on lines
+348
to
+349
| except LookupError: | ||
| raise AttributeError(name, type(self)) |
There was a problem hiding this comment.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Suggested change
| except LookupError: | |
| raise AttributeError(name, type(self)) | |
| except LookupError as e: | |
| raise AttributeError(name, type(self)) from e |
Comment on lines
+30
to
+32
| if e is None: | ||
| return None | ||
| return type(e), e, e.__traceback__ |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Lift code into else after jump in control flow (
reintroduce-else) - Replace if statement with if expression (
assign-if-exp)
Suggested change
| if e is None: | |
| return None | |
| return type(e), e, e.__traceback__ | |
| return None if e is None else (type(e), e, e.__traceback__) |
Comment on lines
61
to
+63
| else: | ||
| ex = self._excinfo | ||
| raise ex[1].with_traceback(ex[2]) | ||
| ex = self._exc | ||
| raise ex.with_traceback(ex.__traceback__) |
There was a problem hiding this comment.
suggestion (code-quality): Remove unnecessary else after guard condition (remove-unnecessary-else)
Suggested change
| else: | |
| ex = self._excinfo | |
| raise ex[1].with_traceback(ex[2]) | |
| ex = self._exc | |
| raise ex.with_traceback(ex.__traceback__) | |
| ex = self._exc | |
| raise ex.with_traceback(ex.__traceback__) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
the goal is to have wrappers and non-wrappers be split into separate lists as well as changing historic hooks
ong term experiment
Summary by Sourcery
Split hook execution into separate wrapper and non-wrapper flows, introduce a unified Result type for hook outcomes, and refactor HookImpl for cleaner argument handling.
Enhancements:
Tests: