From 11955b7765b78bb1cd0aa36d0d1c255a9c999459 Mon Sep 17 00:00:00 2001 From: Tyler Goodlet Date: Mon, 14 Nov 2016 15:39:33 -0500 Subject: [PATCH 1/5] 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 e4444ff4..5ac2518b 100644 --- a/pluggy.py +++ b/pluggy.py @@ -1,5 +1,6 @@ import sys import inspect +import warnings __version__ = '0.5.0' @@ -9,6 +10,14 @@ _py3 = sys.version_info > (3, 0) +class PluginValidationError(Exception): + """ plugin failed validation. """ + + +class HookCallError(Exception): + """ Hook was called wrongly. """ + + class HookspecMarker(object): """ Decorator helper class for marking functions as hook specifications. @@ -266,7 +275,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. @@ -412,14 +423,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 @@ -526,24 +539,25 @@ class _MultiCall(object): # 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: @@ -561,7 +575,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): @@ -581,7 +595,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) @@ -591,10 +605,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: @@ -605,10 +623,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(object): @@ -627,6 +645,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) @@ -638,7 +658,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"): @@ -658,6 +679,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: @@ -679,6 +702,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): @@ -708,6 +738,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) @@ -718,21 +750,13 @@ def _maybe_apply_history(self, method): class HookImpl(object): 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 e27338e49e1401a728210ea7cd9dc278ea0e97a6 Mon Sep 17 00:00:00 2001 From: Tyler Goodlet Date: Tue, 15 Nov 2016 18:37:06 -0500 Subject: [PATCH 2/5] 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 db12f30a..b1780968 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 96b649462ecf4e7f4ca99624eceb5960c98bb1de Mon Sep 17 00:00:00 2001 From: Tyler Goodlet Date: Thu, 16 Feb 2017 02:18:03 -0500 Subject: [PATCH 3/5] Handle py2.6 and better docs - don't use "Positional" in warning message - support python 2.6 sets - don't include __multicall__ in args comparison - document `varnames()` new pair return value --- pluggy.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/pluggy.py b/pluggy.py index 5ac2518b..101e6c6d 100644 --- a/pluggy.py +++ b/pluggy.py @@ -579,11 +579,11 @@ def __repr__(self): def varnames(func): - """Return argument name tuple for a function, method, class or callable. + """Return tuple of positional and keywrord argument names for a function, + method, class or callable. - In case of a class, its "__init__" method is considered. - For methods the "self" parameter is not included unless you are passing - an unbound method with Python3 (which has no support for unbound methods) + In case of a class, its ``__init__`` method is considered. + For methods the ``self`` parameter is not included. """ cache = getattr(func, "__dict__", {}) try: @@ -702,13 +702,15 @@ 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.argnames: + notincall = set(self.argnames) - set(['__multicall__']) - set( + kwargs.keys()) + if notincall: + warnings.warn( + "Argument(s) {0} which are declared in the hookspec " + "can not be found in this hook call" + .format(tuple(notincall)) + ) return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs) def call_historic(self, proc=None, kwargs=None): From 4ab6b1fbd600e1917003b6d0eb5410c45f380320 Mon Sep 17 00:00:00 2001 From: Tyler Goodlet Date: Thu, 16 Feb 2017 02:20:54 -0500 Subject: [PATCH 4/5] Add a call vs. spec mismatch warning test --- testing/test_details.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/testing/test_details.py b/testing/test_details.py index 51280122..e0f3cb9b 100644 --- a/testing/test_details.py +++ b/testing/test_details.py @@ -1,3 +1,4 @@ +import warnings from pluggy import PluginManager, HookimplMarker, HookspecMarker @@ -62,3 +63,33 @@ class Module(object): # register() would raise an error pm.register(module, 'donttouch') assert pm.get_plugin('donttouch') is module + + +def test_warning_on_call_vs_hookspec_arg_mismatch(): + """Verify that is a hook is called with less arguments then defined in the + spec that a warning is emitted. + """ + class Spec: + @hookspec + def myhook(self, arg1, arg2): + pass + + class Plugin: + @hookimpl + def myhook(self, arg1): + pass + + pm = PluginManager(hookspec.project_name) + pm.register(Plugin()) + pm.add_hookspecs(Spec()) + + with warnings.catch_warnings(record=True) as warns: + warnings.simplefilter('always') + + # calling should trigger a warning + pm.hook.myhook(arg1=1) + + assert len(warns) == 1 + warning = warns[-1] + assert issubclass(warning.category, Warning) + assert "Argument(s) ('arg2',)" in str(warning.message) From e5cfb3185582473a178d6b177449e0dd277c1546 Mon Sep 17 00:00:00 2001 From: Tyler Goodlet Date: Sat, 18 Feb 2017 11:30:27 -0500 Subject: [PATCH 5/5] Drop "Positional" adjective --- pluggy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pluggy.py b/pluggy.py index 101e6c6d..fa30fcdb 100644 --- a/pluggy.py +++ b/pluggy.py @@ -428,7 +428,7 @@ def _verify_hook(self, hook, hookimpl): if notinspec: raise PluginValidationError( "Plugin %r for hook %r\nhookimpl definition: %s\n" - "Positional args %s are declared in the hookimpl but " + "Argument(s) %s are declared in the hookimpl but " "can not be found in the hookspec" % (hookimpl.plugin_name, hook.name, _formatdef(hookimpl.function), notinspec)