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. |
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
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! 👍 |
nicoddemus
left a comment
There was a problem hiding this comment.
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.
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.
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).