-
Notifications
You must be signed in to change notification settings - Fork 124
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
Hook wrapping without _Result
#389
Conversation
In #260, @maxnikulin argued against using a "plain" generator function for this, preferring a new My opinion is that implicit is fine, because that's how python functions themselves work, and how e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nicer than what we have
It Will make async a little Harder, but reasonably so
Seems like async generators have the necessary |
Thanks for the reviews @RonnyPfannschmidt. I plan to:
|
Here's a branch converting all of pytest to new-style wrappers: https://github.com/bluetech/pytest/commits/new-style-wrappers All of the tests pass, you can use it to get an impression of usage. I haven't compared performance yet. |
I've made the following performance measurements:
I also made an experiment where I removed |
How expensive would it be to have legacy hook wrappers be implemented in terms off The new wrappers thus making them slower but allowing the new wrappers also Bugfix the exception escape |
I'm not sure, but I generally think it'd be better to keep the existing hookwrapper=True semantics as-is, to avoid breaking things (https://xkcd.com/1172/ situation). Switching to the new style would also be opting in to the new semantics. |
Great work @bluetech! The idea seems great, I will take a look at the code in the next few days! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks @bluetech!
"""Wrap calls to ``setup_project()`` implementations which | ||
should return json encoded config options. | ||
""" | ||
# get initial default config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, the legacy example in Old-style wrappers
could also have the exact same beginning as here: defaults = config.tojson()
, and then printing the debug info via if config.debug
.
(I would suggest that directly below, but I cannot leave a suggestion outside the diff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops I forgot to handle this -- fixing now.
Fix pytest-dev#260. Co-authored-by: Maxim Nikulin <manikulin@gmail.com>
Thanks for the reviews! After taking another look at #257, I've made a few changes:
|
This implements the idea from #260. I think it makes pluggy more elegant, and also provides a backward compatible fix for #244 (replaces #257).