-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
address #8361 - introduce hook caller wrappers that enable backward compat #8463
address #8361 - introduce hook caller wrappers that enable backward compat #8463
Conversation
…backward compat
change makes sense, probably need a test or two to demonstrate the behaviour (do we want a warning and to eventually migrate?) |
I also think this is a good approach to #8361. 👍 |
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.
Thanks for looking at this.
As I mentioned, I don't think we really need to do this, but if we can have a way to deprecate, I guess it's OK.
I think wrapping at the config.hook
level is not sufficient -- some of the hooks use an FSHookProxy
(gethookproxy
) which uses the plugin manager directly. I think if possible it would be better to add this directly on the PytestPluginManager
.
I'm also think this might have some non-negligible effect on performance but I didn't measure. Can do once it's further along.
proxy = FSHookProxy(pm, remove_mods) | ||
from .config.compat import PathAwareHookProxy | ||
|
||
proxy = PathAwareHookProxy(FSHookProxy(pm, remove_mods)) |
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.
@bluetech here i wrap that one
@bluetech i believe, in a followup i can even add some bits to ensure the cost can be removed if people opt out of legacy paths |
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.
Overall great work, please see my comments!
Co-authored-by: Bruno Oliveira <nicoddemus@gmail.com>
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.
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.
LGTM!
fixes #8361