-
Notifications
You must be signed in to change notification settings - Fork 127
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
Howto handle hook changes #170
Comments
this is a unsolved problem as of now - we didn't get the attempted solution to a thing that works pytest also has this issue as the |
@fschulze why can't you add the new argument to the
I actually did implement this and it works (#15 leading to #43) but we never got to consensus on whether it was actually the right way to solve this issue. |
Shoot just realized #138 is only for deprecating full hooks and not specific args. @RonnyPfannschmidt what do you think about something similar like: @hookspec(
warn_on_args={'arg1': DeprecationWarning("arg1 is deprecated and will be removed soon")
)
def oldhook(arg1, new_arg1):
pass |
@tgoodlet doesn't work even remotely since it ignores stuff like caller handling |
@tgoodlet that still wouldn't solve the issue, that the plugin should still work with older "hosts" where the argument wasn't there yet. And I don't want to deprecate the old argument, just introduce an additional one. For me an entirely new hook would be fine, but then I need a way to prevent the old hook to be called in plugins that support the new hook. |
Args are always opt in for hookimpls regardless of whether the spec or call defines more then the impl accepts. Relates to pytest-dev#170
@RonnyPfannschmidt explain? Also what are you referring to with this comment?
@fschulze sorry, why wouldn't this work? To make this work:
I'm not sure I'm following what the problem is and I've opened #172 to demonstrate how you can accomplish this. |
@tgoodlet hook calls are in plugins, now plugins are broken in fun ways as all of them would have a breaking change so the proposed mechanism you showed is one that require breaking api changes |
I'm not following - where's the breakage? This new |
@tgoodlet plug-ins can no longer call hooks as the args changed |
@RonnyPfannschmidt right which is why you first deprecate and then later remove - just like any API progression? If you're going to deprecate and change an API at some point you have to stop supporting it. As per #172 you can definitely keep deprecated and new args working in tandem. To me this is simple:
Implementing this will give good reason to finally bring in #57 as well since we'll want to keep track of which args per |
@tgoodlet all callers must include all arguments - so i have no idea what you are talking about only receivers can leave arguments out |
Yes - no confusion there. So you change the spec to include At some point the calls have to change, I don't see how you can ever get around that. |
@tgoodlet removal isn't the problem here - adding them is |
Why? A call can not declare all args that are in the spec (you'll get a warning) as well as adding a new arg to both the call and spec works just fine with impls which don't declare it, again, as per #170? Besides, |
@tgoodlet the plugins will not have a extra arg, they will have a missing arg to the call and break for example if |
@tgoodlet
As I originally wrote, the plugin wouldn't work with older versions of the app defining the hookspec:
|
The question is where is this changed. If it's in the spec then everything will work since
@RonnyPfannschmidt I'm proposing this emits a warning and the call will still work - again #170. I don't see why it's a problem that plugins do not work on old versions of @fschulze If the spec doesn't define the new args then your call shouldn't either; the plugin should be aware of the host it is dealing with. Trying to hack this inside the call machinery of @RonnyPfannschmidt @fschulze this is the way I see it:
Trying to make this work magically in |
@tgoodlet
I would be fine with that, but at the moment I don't see how the plugin can do that. Do you have an example? Does it work already, or does pluggy need to add some introspection facilities? If my plugin can check whether the host supports a specific hookspec during import, then I could just wrap my hook implementations in an if block and provide either the old implementation, or the new one. I'm not necessarily talking about arguments here, but whole hook functions. On the host I would just do something like myhost_oldhook, myhost_newhook. If myhost_newhook exists, then the plugin wouldn't provide myhost_oldhook. That way my plugin can support the old host without myhost_newhook and on the newer host the myhost_oldhook wouldn't be called for my plugin, but only myhost_newhook. I could already import the hookspec module directly and introspect it, but normally the plugin doesn't know about that module, so I'd like to avoid that. |
im out of this |
😞
@fschulze not yet but like I said #57 would bring it. I just have to dig it all back up. |
Args are always opt in for hookimpls regardless of whether the spec or call defines more then the impl accepts. Relates to pytest-dev#170
Args are always opt in for hookimpls regardless of whether the spec or call defines more then the impl accepts. Relates to pytest-dev#170
Args are always opt in for hookimpls regardless of whether the spec or call defines more then the impl accepts. Relates to pytest-dev#170
Args are always opt in for hookimpls regardless of whether the spec or call defines more then the impl accepts. Relates to pytest-dev#170
Just copying a comment from @fschulze from the #57 PR.
@tgoodlet
That again is from the host side, do you have anything in mind for the plugin side? Is that even possible? I still think my proposal like this would be simpler: Host side: @hookspec(deprecated=True) @hookspec(replaces=myhost_oldhook) Plugin side: @hookimpl(optional=True) @hookimpl(optional=True) Pluggy would then remove myhost_oldhook from the list. This way all the following work:
|
pytest-dev/pytest#8463 as idea for a mechanism |
I have a hook which needs to be changed. In essence it needs an additional argument.
The problem is, that plugins should be able to use the new argument, but still work on older versions where the argument didn't exist yet.
If I add the argument and use it in the hook implementation in the plugin, then the plugin doesn't work with older versions anymore:
Argument(s) ... are declared in the hookimpl but can not be found in the hookspec
Using kwargs with defaults also doesn't work, the argument isn't passed then.
If I add a new hook, then both my implementations in the plugin are called, because the old hook still needs to be called for older plugins.
One idea would be the possibility to mark a hook specification deprecated with the name of the replacement and then when loading a plugin and it implements both hooks forget about the old hook. If it only supports the old hook, then add a warning about the deprecation.
The text was updated successfully, but these errors were encountered: