From f84955310c2aae13d1760948cd85afee3787ee2a Mon Sep 17 00:00:00 2001 From: Tyler Goodlet Date: Mon, 14 Nov 2016 15:39:33 -0500 Subject: [PATCH 1/4] Future warn about hookspec vs. call mis-matches Warn when either a hook call doesn't match a hookspec. Additionally, - Extend `varnames()` to return the list of both the arg and kwarg names for a function - Rename `_MultiCall.kwargs` to `caller_kwargs` to be more explicit - Store hookspec kwargs on `_HookRelay` and `HookImpl` instances Relates to #15 --- pluggy.py | 90 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 57 insertions(+), 33 deletions(-) diff --git a/pluggy.py b/pluggy.py index 08d8c3fc..b7406167 100644 --- a/pluggy.py +++ b/pluggy.py @@ -66,6 +66,7 @@ """ import sys import inspect +import warnings __version__ = '0.5.0' @@ -75,6 +76,14 @@ _py3 = sys.version_info > (3, 0) +class PluginValidationError(Exception): + """ plugin failed validation. """ + + +class HookCallError(Exception): + """ Hook was called wrongly. """ + + class HookspecMarker: """ Decorator helper class for marking functions as hook specifications. @@ -332,7 +341,9 @@ def __init__(self, project_name, implprefix=None): self.hook = _HookRelay(self.trace.root.get("hook")) self._implprefix = implprefix self._inner_hookexec = lambda hook, methods, kwargs: \ - _MultiCall(methods, kwargs, hook.spec_opts).execute() + _MultiCall( + methods, kwargs, specopts=hook.spec_opts, hook=hook + ).execute() def _hookexec(self, hook, methods, kwargs): # called from all hookcaller instances. @@ -478,14 +489,16 @@ def _verify_hook(self, hook, hookimpl): "Plugin %r\nhook %r\nhistoric incompatible to hookwrapper" % (hookimpl.plugin_name, hook.name)) - for arg in hookimpl.argnames: - if arg not in hook.argnames: - raise PluginValidationError( - "Plugin %r\nhook %r\nargument %r not available\n" - "plugin definition: %s\n" - "available hookargs: %s" % - (hookimpl.plugin_name, hook.name, arg, - _formatdef(hookimpl.function), ", ".join(hook.argnames))) + # positional arg checking + notinspec = set(hookimpl.argnames) - set(hook.argnames) + if notinspec: + raise PluginValidationError( + "Plugin %r for hook %r\nhookimpl definition: %s\n" + "Positional args %s are declared in the hookimpl but " + "can not be found in the hookspec" % + (hookimpl.plugin_name, hook.name, + _formatdef(hookimpl.function), notinspec) + ) def check_pending(self): """ Verify that all hooks which have not been verified against @@ -592,24 +605,25 @@ class _MultiCall: # so we can remove it soon, allowing to avoid the below recursion # in execute() and simplify/speed up the execute loop. - def __init__(self, hook_impls, kwargs, specopts={}): + def __init__(self, hook_impls, kwargs, specopts={}, hook=None): + self.hook = hook self.hook_impls = hook_impls - self.kwargs = kwargs - self.kwargs["__multicall__"] = self - self.specopts = specopts + self.caller_kwargs = kwargs # come from _HookCaller.__call__() + self.caller_kwargs["__multicall__"] = self + self.specopts = hook.spec_opts if hook else specopts def execute(self): - all_kwargs = self.kwargs + caller_kwargs = self.caller_kwargs self.results = results = [] firstresult = self.specopts.get("firstresult") while self.hook_impls: hook_impl = self.hook_impls.pop() try: - args = [all_kwargs[argname] for argname in hook_impl.argnames] + args = [caller_kwargs[argname] for argname in hook_impl.argnames] except KeyError: for argname in hook_impl.argnames: - if argname not in all_kwargs: + if argname not in caller_kwargs: raise HookCallError( "hook call must provide argument %r" % (argname,)) if hook_impl.hookwrapper: @@ -627,7 +641,7 @@ def __repr__(self): status = "%d meths" % (len(self.hook_impls),) if hasattr(self, "results"): status = ("%d results, " % len(self.results)) + status - return "<_MultiCall %s, kwargs=%r>" % (status, self.kwargs) + return "<_MultiCall %s, kwargs=%r>" % (status, self.caller_kwargs) def varnames(func): @@ -647,7 +661,7 @@ def varnames(func): try: func = func.__init__ except AttributeError: - return () + return (), () elif not inspect.isroutine(func): # callable object? try: func = getattr(func, '__call__', func) @@ -657,10 +671,14 @@ def varnames(func): try: # func MUST be a function or method here or we won't parse any args spec = inspect.getargspec(func) except TypeError: - return () + return (), () - args, defaults = spec.args, spec.defaults - args = args[:-len(defaults)] if defaults else args + args, defaults = tuple(spec.args), spec.defaults + if defaults: + index = -len(defaults) + args, defaults = args[:index], tuple(args[index:]) + else: + defaults = () # strip any implicit instance arg if args: @@ -671,10 +689,10 @@ def varnames(func): assert "self" not in args # best naming practises check? try: - cache["_varnames"] = args + cache["_varnames"] = args, defaults except TypeError: pass - return tuple(args) + return args, defaults class _HookRelay: @@ -693,6 +711,8 @@ def __init__(self, name, hook_execute, specmodule_or_class=None, spec_opts=None) self._wrappers = [] self._nonwrappers = [] self._hookexec = hook_execute + self.argnames = None + self.kwargnames = None if specmodule_or_class is not None: assert spec_opts is not None self.set_specification(specmodule_or_class, spec_opts) @@ -704,7 +724,8 @@ def set_specification(self, specmodule_or_class, spec_opts): assert not self.has_spec() self._specmodule_or_class = specmodule_or_class specfunc = getattr(specmodule_or_class, self.name) - argnames = varnames(specfunc) + # get spec arg signature + argnames, self.kwargnames = varnames(specfunc) self.argnames = ["__multicall__"] + list(argnames) self.spec_opts = spec_opts if spec_opts.get("historic"): @@ -724,6 +745,8 @@ def remove(wrappers): raise ValueError("plugin %r not found" % (plugin,)) def _add_hookimpl(self, hookimpl): + """A an implementation to the callback chain. + """ if hookimpl.hookwrapper: methods = self._wrappers else: @@ -745,6 +768,13 @@ def __repr__(self): def __call__(self, **kwargs): assert not self.is_historic() + notincall = set(self.argnames) - set(kwargs.keys()) + if notincall: + warnings.warn( + "Positional arg(s) %s are declared in the hookspec " + "but can not be found in this hook call" % notincall, + FutureWarning + ) return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs) def call_historic(self, proc=None, kwargs=None): @@ -774,6 +804,8 @@ def call_extra(self, methods, kwargs): self._nonwrappers, self._wrappers = old def _maybe_apply_history(self, method): + """Apply call history to a new hookimpl if it is marked as historic. + """ if self.is_historic(): for kwargs, proc in self._call_history: res = self._hookexec(self, [method], kwargs) @@ -784,21 +816,13 @@ def _maybe_apply_history(self, method): class HookImpl: def __init__(self, plugin, plugin_name, function, hook_impl_opts): self.function = function - self.argnames = varnames(self.function) + self.argnames, self.kwargnames = varnames(self.function) self.plugin = plugin self.opts = hook_impl_opts self.plugin_name = plugin_name self.__dict__.update(hook_impl_opts) -class PluginValidationError(Exception): - """ plugin failed validation. """ - - -class HookCallError(Exception): - """ Hook was called wrongly. """ - - if hasattr(inspect, 'signature'): def _formatdef(func): return "%s%s" % ( From 22f3b0a5c8f37ad4cf42be0c48a637b5b8b85008 Mon Sep 17 00:00:00 2001 From: Tyler Goodlet Date: Tue, 15 Nov 2016 18:37:06 -0500 Subject: [PATCH 2/4] Port tests to match new `varnames()` return value --- testing/test_helpers.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/testing/test_helpers.py b/testing/test_helpers.py index bb813bcf..2f332252 100644 --- a/testing/test_helpers.py +++ b/testing/test_helpers.py @@ -13,16 +13,16 @@ class B(object): def __call__(self, z): pass - assert varnames(f) == ("x",) - assert varnames(A().f) == ('y',) - assert varnames(B()) == ('z',) + assert varnames(f) == (("x",), ()) + assert varnames(A().f) == (('y',), ()) + assert varnames(B()) == (('z',), ()) def test_varnames_default(): def f(x, y=3): pass - assert varnames(f) == ("x",) + assert varnames(f) == (("x",), ("y",)) def test_varnames_class(): @@ -40,10 +40,10 @@ def __init__(self, x): class F(object): pass - assert varnames(C) == ("x",) - assert varnames(D) == () - assert varnames(E) == ("x",) - assert varnames(F) == () + assert varnames(C) == (("x",), ()) + assert varnames(D) == ((), ()) + assert varnames(E) == (("x",), ()) + assert varnames(F) == ((), ()) def test_formatdef(): From d743939dd98c839ea9a498f781b114ae74a67f03 Mon Sep 17 00:00:00 2001 From: Tyler Goodlet Date: Thu, 17 Nov 2016 18:52:58 -0500 Subject: [PATCH 3/4] Add defaults support to enable deprecation Add support for using declared keyword argument values from both hookspecs and hookimpls. The current logic will inspect the hookimpl and, if it contains defaults, values will be first looked up from the caller provided data and if not defined will be taken from the hookspec's declared defaults. If the spec does not define defaults the value is taken from the hookimpl's defaults as is expected under normal function call semantics. Resolves #15 --- pluggy.py | 89 ++++++++++++++++++++++----------- testing/test_method_ordering.py | 6 +-- testing/test_multicall.py | 2 +- 3 files changed, 65 insertions(+), 32 deletions(-) diff --git a/pluggy.py b/pluggy.py index b7406167..ff34b8b0 100644 --- a/pluggy.py +++ b/pluggy.py @@ -66,6 +66,7 @@ """ import sys import inspect +import copy import warnings __version__ = '0.5.0' @@ -340,10 +341,12 @@ def __init__(self, project_name, implprefix=None): self.trace = _TagTracer().get("pluginmanage") self.hook = _HookRelay(self.trace.root.get("hook")) self._implprefix = implprefix - self._inner_hookexec = lambda hook, methods, kwargs: \ - _MultiCall( - methods, kwargs, specopts=hook.spec_opts, hook=hook - ).execute() + self._inner_hookexec = lambda hook, methods, kwargs: _MultiCall( + methods, + kwargs, + firstresult=hook.spec.opts['firstresult'] if hook.spec else False, + hook=hook + ).execute() def _hookexec(self, hook, methods, kwargs): # called from all hookcaller instances. @@ -490,7 +493,7 @@ def _verify_hook(self, hook, hookimpl): (hookimpl.plugin_name, hook.name)) # positional arg checking - notinspec = set(hookimpl.argnames) - set(hook.argnames) + notinspec = set(hookimpl.argnames) - set(hook.spec.argnames) if notinspec: raise PluginValidationError( "Plugin %r for hook %r\nhookimpl definition: %s\n" @@ -583,8 +586,8 @@ def subset_hook_caller(self, name, remove_plugins): orig = getattr(self.hook, name) plugins_to_remove = [plug for plug in remove_plugins if hasattr(plug, name)] if plugins_to_remove: - hc = _HookCaller(orig.name, orig._hookexec, orig._specmodule_or_class, - orig.spec_opts) + hc = _HookCaller(orig.name, orig._hookexec, orig.spec.namespace, + orig.spec.opts) for hookimpl in (orig._wrappers + orig._nonwrappers): plugin = hookimpl.plugin if plugin not in plugins_to_remove: @@ -605,29 +608,43 @@ class _MultiCall: # so we can remove it soon, allowing to avoid the below recursion # in execute() and simplify/speed up the execute loop. - def __init__(self, hook_impls, kwargs, specopts={}, hook=None): - self.hook = hook + def __init__(self, hook_impls, kwargs, firstresult=False, hook=None): self.hook_impls = hook_impls self.caller_kwargs = kwargs # come from _HookCaller.__call__() self.caller_kwargs["__multicall__"] = self - self.specopts = hook.spec_opts if hook else specopts + self.firstresult = firstresult + self.hook = hook + self.spec = hook.spec if hook else None def execute(self): caller_kwargs = self.caller_kwargs self.results = results = [] - firstresult = self.specopts.get("firstresult") + firstresult = self.firstresult + spec = self.spec while self.hook_impls: hook_impl = self.hook_impls.pop() + implkwargs = hook_impl.kwargs try: args = [caller_kwargs[argname] for argname in hook_impl.argnames] + # get any caller provided kwargs declared in our + # hookimpl and fail over to the spec's value if provided + if implkwargs: + kwargs = copy.copy(implkwargs) + if spec: + kwargs.update(spec.kwargs) + + args += [caller_kwargs.get(argname, kwargs[argname]) + for argname in hook_impl.kwargnames] except KeyError: for argname in hook_impl.argnames: if argname not in caller_kwargs: raise HookCallError( "hook call must provide argument %r" % (argname,)) + if hook_impl.hookwrapper: return _wrapped_call(hook_impl.function(*args), self.execute) + res = hook_impl.function(*args) if res is not None: if firstresult: @@ -711,28 +728,23 @@ def __init__(self, name, hook_execute, specmodule_or_class=None, spec_opts=None) self._wrappers = [] self._nonwrappers = [] self._hookexec = hook_execute - self.argnames = None - self.kwargnames = None + self.spec = None + self._call_history = None if specmodule_or_class is not None: assert spec_opts is not None self.set_specification(specmodule_or_class, spec_opts) def has_spec(self): - return hasattr(self, "_specmodule_or_class") + return self.spec is not None def set_specification(self, specmodule_or_class, spec_opts): assert not self.has_spec() - self._specmodule_or_class = specmodule_or_class - specfunc = getattr(specmodule_or_class, self.name) - # get spec arg signature - argnames, self.kwargnames = varnames(specfunc) - self.argnames = ["__multicall__"] + list(argnames) - self.spec_opts = spec_opts + self.spec = HookSpec(specmodule_or_class, self.name, spec_opts) if spec_opts.get("historic"): self._call_history = [] def is_historic(self): - return hasattr(self, "_call_history") + return self._call_history is not None def _remove_plugin(self, plugin): def remove(wrappers): @@ -768,13 +780,14 @@ def __repr__(self): def __call__(self, **kwargs): assert not self.is_historic() - notincall = set(self.argnames) - set(kwargs.keys()) - if notincall: - warnings.warn( - "Positional arg(s) %s are declared in the hookspec " - "but can not be found in this hook call" % notincall, - FutureWarning - ) + if self.spec: + notincall = set(self.spec.argnames) - set(kwargs.keys()) + if notincall: + warnings.warn( + "Positional arg(s) %s are declared in the hookspec " + "but can not be found in this hook call" % notincall, + FutureWarning + ) return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs) def call_historic(self, proc=None, kwargs=None): @@ -813,10 +826,30 @@ def _maybe_apply_history(self, method): proc(res[0]) +class HookSpec: + def __init__(self, namespace, name, hook_spec_opts): + self.namespace = namespace + self.function = function = getattr(namespace, name) + self.name = name + self.argnames, self.kwargnames = varnames(function) + self.kwargvalues = inspect.getargspec(function).defaults + self.kwargs = dict( + ((name, value) for name, value in + zip(self.kwargnames, inspect.getargspec(function).defaults)) + ) if self.kwargvalues else {} + self.opts = hook_spec_opts + self.argnames = ["__multicall__"] + list(self.argnames) + + class HookImpl: def __init__(self, plugin, plugin_name, function, hook_impl_opts): self.function = function self.argnames, self.kwargnames = varnames(self.function) + self.kwargvalues = inspect.getargspec(function).defaults + self.kwargs = dict( + ((name, value) for name, value in + zip(self.kwargnames, inspect.getargspec(function).defaults)) + ) if self.kwargvalues else {} self.plugin = plugin self.opts = hook_impl_opts self.plugin_name = plugin_name diff --git a/testing/test_method_ordering.py b/testing/test_method_ordering.py index c8bd39b1..56fcb045 100644 --- a/testing/test_method_ordering.py +++ b/testing/test_method_ordering.py @@ -163,9 +163,9 @@ def he_myhook3(arg1): pass pm.add_hookspecs(HookSpec) - assert not pm.hook.he_myhook1.spec_opts["firstresult"] - assert pm.hook.he_myhook2.spec_opts["firstresult"] - assert not pm.hook.he_myhook3.spec_opts["firstresult"] + assert not pm.hook.he_myhook1.spec.opts["firstresult"] + assert pm.hook.he_myhook2.spec.opts["firstresult"] + assert not pm.hook.he_myhook3.spec.opts["firstresult"] @pytest.mark.parametrize('name', ["hookwrapper", "optionalhook", "tryfirst", "trylast"]) diff --git a/testing/test_multicall.py b/testing/test_multicall.py index 0d668049..a0890f52 100644 --- a/testing/test_multicall.py +++ b/testing/test_multicall.py @@ -22,7 +22,7 @@ def MC(methods, kwargs, firstresult=False): for method in methods: f = HookImpl(None, "", method, method.example_impl) hookfuncs.append(f) - return _MultiCall(hookfuncs, kwargs, {"firstresult": firstresult}) + return _MultiCall(hookfuncs, kwargs, firstresult=firstresult) def test_call_passing(): From 178ffdf280d404e7ebbe7aebab3354318a8cb4b1 Mon Sep 17 00:00:00 2001 From: Tyler Goodlet Date: Sat, 19 Nov 2016 12:46:47 -0500 Subject: [PATCH 4/4] Add a defaults precedence test Verify that defaults declared in hook impls and specs adhere to the lookup order: call provided value, hook spec default, and finally falling back to the spec's default value. --- testing/test_hookrelay.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/testing/test_hookrelay.py b/testing/test_hookrelay.py index a44a5fc4..36dfff2a 100644 --- a/testing/test_hookrelay.py +++ b/testing/test_hookrelay.py @@ -76,3 +76,32 @@ def hello(self, arg): pm.register(Plugin()) res = pm.hook.hello(arg=3) assert res == 4 + + +def test_defaults(pm): + """Verify that default keyword arguments can be declared on both specs + and impls. The default value look up precedence is up as follows: + - caller provided value + - hookspec default + - hookimpl default + """ + class Api: + @hookspec + def myhook(self, arg, kwarg="default"): + "A spec with a default" + + class Plugin: + @hookimpl + def myhook(self, arg, kwarg="my default"): + return kwarg + + pm.register(Plugin()) + + # with no spec registered + assert pm.hook.myhook(arg='yeah!')[0] == "my default" + assert pm.hook.myhook(arg='yeah!', kwarg='doggy')[0] == "doggy" + + # with spec registered + pm.add_hookspecs(Api) + assert pm.hook.myhook(arg='yeah!')[0] == "default" + assert pm.hook.myhook(arg='yeah!', kwarg='doggy')[0] == "doggy"