Skip to content

refactor: implement legacy hookwrappers in terms of a modern hook" #546

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

RonnyPfannschmidt
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt commented Nov 3, 2024

followup to #545

moves the implementation of legacy hooks into a new style hook

@RonnyPfannschmidt
Copy link
Member Author

@nicoddemus ping - can you take a look

@RonnyPfannschmidt
Copy link
Member Author

failures unrelated as pytest dropped python3.8 on main

@RonnyPfannschmidt RonnyPfannschmidt force-pushed the ronny/hookwrapper-wrap-legacy branch 2 times, most recently from ae3572d to a3c8d56 Compare December 21, 2024 10:28
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

LGTM!

Sorry for the delay, seems like I missed the notifications for this one.

Teardown = Generator[None, object, object]


def run_legacy_hookwrapper(
Copy link
Member

Choose a reason for hiding this comment

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

We call this "old-style" in the docs, perhaps:

Suggested change
def run_legacy_hookwrapper(
def run_old_style_hookwrapper(

@RonnyPfannschmidt RonnyPfannschmidt force-pushed the ronny/hookwrapper-wrap-legacy branch 2 times, most recently from 9376cb5 to 903653c Compare March 16, 2025 20:02
@RonnyPfannschmidt
Copy link
Member Author

@nicoddemus the coverage seems bogous

Raising a StopIteration in a generator triggers a RuntimeError.
If the RuntimeError of a generator has the passed in StopIteration as cause
resume with that StopIteration as normal exception instead of failing with the RuntimeError.
@RonnyPfannschmidt RonnyPfannschmidt force-pushed the ronny/hookwrapper-wrap-legacy branch from 903653c to 25af101 Compare March 27, 2025 10:22
@RonnyPfannschmidt RonnyPfannschmidt force-pushed the ronny/hookwrapper-wrap-legacy branch from a4e03dc to d0ab361 Compare April 9, 2025 20:53
@RonnyPfannschmidt RonnyPfannschmidt force-pushed the ronny/hookwrapper-wrap-legacy branch from 16114aa to d443764 Compare April 10, 2025 10:42
@RonnyPfannschmidt
Copy link
Member Author

@nicoddemus i took a rather brutal approach to coverage - please have another look

@RonnyPfannschmidt
Copy link
Member Author

@nicoddemus gentle ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants