diff --git a/changelog/170.feature.rst b/changelog/170.feature.rst new file mode 100644 index 00000000..761dbad2 --- /dev/null +++ b/changelog/170.feature.rst @@ -0,0 +1,3 @@ +Hookspecs can now delcare default values for arguments, using the usual ``def myhook(myarg="default")`` syntax. +This is mainly a backward compatibility feature, to allow a hookspec to add new parameters to an existing hook without breaking existing *callers*. +See :ref:`hookspec_arg_default` for details. diff --git a/docs/index.rst b/docs/index.rst index b56278ed..9d56f019 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -563,6 +563,8 @@ behavior you can run a check with the to enforce requisite *hookspecs* but with certain exceptions for some hooks then make sure to mark those hooks as :ref:`optional `. +.. _opt_in_arguments: + Opt-in arguments ^^^^^^^^^^^^^^^^ To allow for *hookspecs* to evolve over the lifetime of a project, @@ -608,6 +610,37 @@ whereas this is not: many arguments as its *hookimpls*) is the conventional :ref:`self ` arg; this is always ignored when *hookimpls* are defined as :ref:`methods `. + +.. _hookspec_arg_default: + +Hookspec argument defaults +^^^^^^^^^^^^^^^^^^^^^^^^^^ +A host program can specify hooks that are intended to be called by external +plugins it has no control of, and not just by itself. This adds another +dimension of backward compatibility to consider when evolving a hookspec. + +While :ref:`opt-in arguments ` allows a hookspec to evolve +while keeping existing *hookimpls* working, it doesn't help with existing *hook +callers*. To handle this, pluggy allows a hookspec to declare a default value +for an argument, using the usual ``def myhook(new_arg="default")`` Python syntax. + +If a caller omits this argument, pluggy will supply the default to the hookimpls. +This allows the hookspec to add the new argument without breaking existing +callers: + +.. code-block:: python + + @hookspec + def myhook(config, args, new_arg=None): + pass + + + # Existing caller; hookimpls will get new_arg=None. + pm.hook.myhook(config=config, args=args) + # New caller; hookimpls will get new_arg="get this". + pm.hook.myhook(config=config, args=args, new_arg="get this") + + .. _firstresult: First result only diff --git a/src/pluggy/_hooks.py b/src/pluggy/_hooks.py index 1480308e..3c1eaaaa 100644 --- a/src/pluggy/_hooks.py +++ b/src/pluggy/_hooks.py @@ -505,6 +505,11 @@ def _add_hookimpl(self, hookimpl: HookImpl) -> None: def __repr__(self) -> str: return f"" + def _apply_defaults(self, kwargs: Mapping[str, object]) -> Mapping[str, object]: + if self.spec is None or not self.spec.kwargdefaults: + return kwargs + return {**self.spec.kwargdefaults, **kwargs} + def _verify_all_args_are_provided(self, kwargs: Mapping[str, object]) -> None: # This is written to avoid expensive operations when not needed. if self.spec: @@ -539,10 +544,13 @@ def __call__(self, **kwargs: object) -> Any: assert not self.is_historic(), ( "Cannot directly call a historic hook - use call_historic instead." ) - self._verify_all_args_are_provided(kwargs) + call_kwargs = self._apply_defaults(kwargs) + self._verify_all_args_are_provided(call_kwargs) firstresult = self.spec.opts.get("firstresult", False) if self.spec else False # Copy because plugins may register other plugins during iteration (#438). - return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult) + return self._hookexec( + self.name, self._hookimpls.copy(), call_kwargs, firstresult + ) def call_historic( self, @@ -559,6 +567,7 @@ def call_historic( """ assert self._call_history is not None kwargs = kwargs or {} + kwargs = self._apply_defaults(kwargs) self._verify_all_args_are_provided(kwargs) self._call_history.append((kwargs, result_callback)) # Historizing hooks don't return results. @@ -580,6 +589,7 @@ def call_extra( assert not self.is_historic(), ( "Cannot directly call a historic hook - use call_historic instead." ) + kwargs = self._apply_defaults(kwargs) self._verify_all_args_are_provided(kwargs) opts: HookimplOpts = { "wrapper": False, @@ -729,6 +739,7 @@ class HookSpec: __slots__ = ( "argnames", "function", + "kwargdefaults", "kwargnames", "name", "namespace", @@ -747,6 +758,8 @@ def __init__(self, namespace: _Namespace, name: str, opts: HookspecOpts) -> None self.argnames, self.kwargnames = varnames( self.function, legacy_noself=legacy_noself ) + defaults = inspect.unwrap(self.function).__defaults__ + self.kwargdefaults = dict(zip(self.kwargnames, defaults or ())) self.opts = opts self.warn_on_impl = opts.get("warn_on_impl") self.warn_on_impl_args = opts.get("warn_on_impl_args") diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 06923c20..448d5af3 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -414,7 +414,9 @@ def _verify_hook(self, hook: HookCaller, hookimpl: HookImpl) -> None: _warn_for_function(hook.spec.warn_on_impl, hookimpl.function) # positional arg checking - notinspec = set(hookimpl.argnames) - set(hook.spec.argnames) + notinspec = set(hookimpl.argnames) - ( + set(hook.spec.argnames) | set(hook.spec.kwargnames) + ) if notinspec: raise PluginValidationError( hookimpl.plugin, diff --git a/testing/test_invocations.py b/testing/test_invocations.py index e24af750..f67b0eb9 100644 --- a/testing/test_invocations.py +++ b/testing/test_invocations.py @@ -367,3 +367,193 @@ def wrap(self): pm.register(Plugin()) with pytest.raises(RuntimeError, match="wrap_controller at 'wrap'.* did not yield"): pm.hook.wrap() + + +class TestHookspecDefaultArgumentValue: + """Tests for the hookspec functionality of providing a default value for args.""" + + def test_basic(self, pm: PluginManager) -> None: + """Basic scenario with old and new impls.""" + default = object() + + class Api: + @hookspec + def hello(self, arg, new_arg=default): + pass + + class NewPlugin: + @hookimpl + def hello(self, arg, new_arg): + return "new", arg, new_arg + + class OldPlugin: + @hookimpl + def hello(self, arg): + return "old", arg + + pm.add_hookspecs(Api) + pm.register(NewPlugin()) + pm.register(OldPlugin()) + + assert pm.hook.hello(arg=1) == [("old", 1), ("new", 1, default)] + + assert pm.hook.hello(arg=1, new_arg="call") == [("old", 1), ("new", 1, "call")] + + def test_added_after_registration(self, pm: PluginManager) -> None: + """If spec is registered after plugin, the default still works.""" + + class Plugin: + @hookimpl + def hello(self, arg, new_arg): + return arg, new_arg + + pm.register(Plugin()) + + class Api: + @hookspec + def hello(self, arg, new_arg="default"): + pass + + pm.add_hookspecs(Api) + + assert pm.hook.hello(arg=1) == [(1, "default")] + + def test_old_spec_new_impl(self, pm: PluginManager) -> None: + """Test what happens when old spec version is used with new impl version. + + The impl must provide its *own* default if it wants to keep supporting + old spec versions. + """ + + class Api: + @hookspec + def hello(self, arg): + pass + + pm.add_hookspecs(Api) + + class PluginWithoutImplDefault: + @hookimpl + def hello(self, arg, new_arg): + return arg, new_arg # pragma: no cover + + class PluginWithImplDefault: + @hookimpl + def hello(self, arg, new_arg="impl-default"): + return arg, new_arg + + with pytest.raises(PluginValidationError): + pm.register(PluginWithoutImplDefault()) + pm.register(PluginWithImplDefault()) + + assert pm.hook.hello(arg=1) == [(1, "impl-default")] + + def test_does_not_override_hookimpl_default(self, pm: PluginManager) -> None: + """If an impl provides its own default, it takes precedence over both + the spec default and call value. + + NOTE: This is verifying existing behavior, but it's not necessarily what + we want (#442). + """ + + class Api: + @hookspec + def hello(self, arg, new_arg="spec-default"): + pass + + class Plugin: + @hookimpl + def hello(self, arg, new_arg="impl-default"): + return new_arg + + pm.add_hookspecs(Api) + pm.register(Plugin()) + + assert pm.hook.hello(arg=1) == ["impl-default"] + assert pm.hook.hello(arg=1, new_arg="call") == ["impl-default"] + + def test_new_call_argument_is_not_delivered_by_old_spec( + self, pm: PluginManager + ) -> None: + """If call uses new spec, but both spec and plugin are old, the new arg + is just ignored.""" + + class Api: + @hookspec + def hello(self, arg): + pass + + class Plugin: + @hookimpl + def hello(self, arg): + return arg + + pm.add_hookspecs(Api) + pm.register(Plugin()) + + assert pm.hook.hello(arg=1, extra=2) == [1] + + @pytest.mark.parametrize("value", [None, False, 0, ""]) + def test_falsy_values_allowed_as_default( + self, pm: PluginManager, value: object + ) -> None: + """Make sure falsy values like None are allowed as default (e.g. not + used as special sentinels by the implementation).""" + + class Api: + @hookspec + def hello(self, arg=value): + pass + + class Plugin: + @hookimpl + def hello(self, arg): + return (arg,) + + pm.add_hookspecs(Api) + pm.register(Plugin()) + + assert pm.hook.hello() == [(value,)] + assert pm.hook.hello(arg=value) == [(value,)] + + def test_historic_replay(self, pm: PluginManager) -> None: + """Test historic calls respect defaults.""" + + class Api: + @hookspec(historic=True) + def hello(self, arg, new_arg="spec"): + pass + + pm.add_hookspecs(Api) + + pm.hook.hello.call_historic(kwargs={"arg": 1}) + pm.hook.hello.call_historic(kwargs={"arg": 2, "new_arg": "call"}) + + seen = [] + + class Plugin: + @hookimpl + def hello(self, arg, new_arg): + seen.append((arg, new_arg)) + + pm.register(Plugin()) + + assert seen == [(1, "spec"), (2, "call")] + + def test_call_extra(self, pm: PluginManager) -> None: + """Test calls with extras respect defaults.""" + + class Api: + @hookspec + def hello(self, arg, new_arg="spec"): + pass + + pm.add_hookspecs(Api) + + def hello(arg, new_arg): + return arg, new_arg + + assert pm.hook.hello.call_extra([hello], {"arg": 1}) == [(1, "spec")] + assert pm.hook.hello.call_extra([hello], {"arg": 2, "new_arg": "call"}) == [ + (2, "call") + ]