Skip to content

Commit

Permalink
Hook call mismatch warnings (pytest-dev#42)
Browse files Browse the repository at this point in the history
* 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 pytest-dev#15

* Port tests to match new `varnames()` return value

* 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

* Add a call vs. spec mismatch warning test

* Drop "Positional" adjective
  • Loading branch information
goodboy authored and nicoddemus committed Feb 25, 2017
1 parent cf059aa commit 26ddfe4
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 45 deletions.
100 changes: 63 additions & 37 deletions pluggy.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import sys
import inspect
import warnings

__version__ = '0.5.0'

Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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"
"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)
)

def check_pending(self):
""" Verify that all hooks which have not been verified against
Expand Down Expand Up @@ -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:
Expand All @@ -561,15 +575,15 @@ 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):
"""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:
Expand All @@ -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)
Expand All @@ -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:
Expand All @@ -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):
Expand All @@ -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)
Expand All @@ -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"):
Expand All @@ -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:
Expand All @@ -679,6 +702,15 @@ def __repr__(self):

def __call__(self, **kwargs):
assert not self.is_historic()
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):
Expand Down Expand Up @@ -708,6 +740,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)
Expand All @@ -718,21 +752,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" % (
Expand Down
31 changes: 31 additions & 0 deletions testing/test_details.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import warnings
from pluggy import PluginManager, HookimplMarker, HookspecMarker


Expand Down Expand Up @@ -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)
16 changes: 8 additions & 8 deletions testing/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand All @@ -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():
Expand Down

0 comments on commit 26ddfe4

Please sign in to comment.