diff --git a/source/extensionPoints/util.py b/source/extensionPoints/util.py index c07f180d3c4..d8fae66ae47 100644 --- a/source/extensionPoints/util.py +++ b/source/extensionPoints/util.py @@ -82,10 +82,11 @@ def register(self, handler): However, the callable must be kept alive by your code otherwise it will be de-registered. This is due to the use of weak references. This is especially relevant when using lambdas. """ - # #9720 (Py3 review required): this method causes unittest to fail in Python 3. - if hasattr(handler, "__self__"): - if not handler.__self__: + if inspect.isfunction(handler): + sig = inspect.signature(handler) + if sig.parameters and list(sig.parameters)[0] == "self": raise TypeError("Registering unbound instance methods not supported.") + if inspect.ismethod(handler): weak = BoundMethodWeakref(handler, self.unregister) else: weak = AnnotatableWeakref(handler, self.unregister) @@ -128,7 +129,10 @@ def callWithSupportedKwargs(func, *args, **kwargs): Instead of raising a TypeError, myFunc will simply be called like this: C{myFunc(a=1, b=2)} - While C{callWithSupportedKwargs} does support positional arguments (C{*args}), usage is strongly discouraged due to the + C{callWithSupportedKwargs} does support positional arguments (C{*args}). + Unfortunately, positional args can not be matched on name (keyword) + to the names of the params in the handler. + Therefore, usage is strongly discouraged due to the risk of parameter order differences causing bugs. @param func: can be any callable that is not an unbound method. EG: @@ -137,6 +141,7 @@ def callWithSupportedKwargs(func, *args, **kwargs): - static methods - functions - lambdas + - partials The arguments for the supplied callable, C{func}, do not need to have default values, and can take C{**kwargs} to capture all arguments. @@ -146,46 +151,22 @@ def callWithSupportedKwargs(func, *args, **kwargs): - the number of positional arguments given can not be received by C{func}. - parameters required (parameters declared with no default value) by C{func} are not supplied. """ - spec = inspect.getargspec(func) - - # some handlers are instance/class methods, discard "self"/"cls" because it is typically passed implicitly. - if inspect.ismethod(func): - spec.args.pop(0) # remove "self"/"cls" for instance methods - if not hasattr(func, "__self__"): - raise TypeError("Unbound instance methods are not handled.") - - # Ensure that the positional args provided by the caller of `callWithSupportedKwargs` actually have a place to go. - # Unfortunately, positional args can not be matched on name (keyword) to the names of the params in the handler, - # and so calling `callWithSupportedKwargs` is at risk of causing bugs if parameter order differs. - numExpectedArgs = len(spec.args) - numGivenPositionalArgs = len(args) - if numGivenPositionalArgs > numExpectedArgs: - raise TypeError("Expected to be able to pass {} positional arguments.".format(numGivenPositionalArgs)) - - # Ensure that all arguments without defaults which are expected by the handler were provided. - # `defaults` is a tuple of default argument values or None if there are no default arguments; - # if this tuple has N elements, they correspond to the last N elements listed in args. - numExpectedArgsWithDefaults = len(spec.defaults) if spec.defaults else 0 - if not spec.defaults or numExpectedArgsWithDefaults != numExpectedArgs: - # get the names of the args without defaults, skipping the N positional args given to `callWithSupportedKwargs` - # positionals are required for the Filter extension point. - # #9067 (Py3 review required): Set construction will work with iterators, too. - givenKwargsKeys = set(kwargs.keys()) - firstArgWithDefault = numExpectedArgs - numExpectedArgsWithDefaults - specArgs = set(spec.args[numGivenPositionalArgs:firstArgWithDefault]) - for arg in specArgs: - # and ensure they are in the kwargs list - if arg not in givenKwargsKeys: - raise TypeError("Parameter required for handler not provided: {}".format(arg)) - - if spec.keywords: - # func has a catch-all for kwargs (**kwargs) so we do not need to filter to just the supported args. - return func(*args, **kwargs) - - supportedKwargs = set(spec.args) - # #9067 (Py3 review required): originally called dict.keys. - # Therefore wrap this inside a list call. - for kwarg in list(kwargs.keys()): - if kwarg not in supportedKwargs: - del kwargs[kwarg] - return func(*args, **kwargs) \ No newline at end of file + sig = inspect.signature(func) + + if inspect.isfunction(func) and sig.parameters and list(sig.parameters)[0] == "self": + raise TypeError("Unbound instance methods are not handled.") + + # Check whether func has a catch-all for kwargs (**kwargs) + # In this case, we do not need to filter to just the supported args. + if not any( + param for param in sig.parameters.values() + if param.kind == param.VAR_KEYWORD + ): + # Delete all the kwargs that are not supported by this callable. + # Wrap the items call in a list, as the dictionary changes during iteration. + for kwarg in list(kwargs.keys()): + if kwarg not in sig.parameters: + del kwargs[kwarg] + + boundArguments = sig.bind(*args, **kwargs) + return func(*boundArguments.args, **boundArguments.kwargs) diff --git a/tests/unit/test_extensionPoints.py b/tests/unit/test_extensionPoints.py index b8f7590202a..9bed87b295a 100644 --- a/tests/unit/test_extensionPoints.py +++ b/tests/unit/test_extensionPoints.py @@ -2,13 +2,14 @@ #A part of NonVisual Desktop Access (NVDA) #This file is covered by the GNU General Public License. #See the file COPYING for more details. -#Copyright (C) 2017-2019 NV Access Limited +#Copyright (C) 2017-2019 NV Access Limited, Leonard de Ruijter """Unit tests for the extensionPoints module. """ import unittest import extensionPoints +from functools import partial class ExampleClass(object): def method(self): @@ -142,6 +143,22 @@ def handlerMethod(self, a): extensionPoints.callWithSupportedKwargs(h.handlerMethod, 'a value') self.assertEqual(calledKwargs, {'a': 'a value'}) + def test_instanceMethodHandlerTakesParams_givenRequiredKwarg(self): + """Test to ensure that a instance method handler gets the correct arguments, including implicit "self" + Handler takes a required keyword argument. + callWithSupportedKwargs given a keyword arg with a matching name. + Handler should get required kwarg. + """ + calledKwargs = {} + + class handlerClass(): + def handlerMethod(self, *, a): + calledKwargs['a'] = a + + h = handlerClass() + extensionPoints.callWithSupportedKwargs(h.handlerMethod, a='a value') + self.assertEqual(calledKwargs, {'a': 'a value'}) + def test_instanceMethodHandlerTakesParams_givenMatchingNameKwarg(self): """Test to ensure that a instance method handler gets the correct arguments, including implicit "self" Handler takes a parameter. @@ -242,7 +259,6 @@ def handler(a): with self.assertRaises(TypeError): extensionPoints.callWithSupportedKwargs(handler, b='b value') - def test_handlerTakesTwoParamsWithoutDefaults_NotEnoughPositionalsGiven_exceptionRaised(self): """ Tests that handlers that when a handler expects params which are not provided, then the function is not called. The handler function takes a param with no default value set. @@ -335,8 +351,6 @@ def test_registerInstanceMethod(self): actual = list(self.reg.handlers) self.assertEqual(actual, [inst.method]) - # #9720 (Py3 review required): for some reason, this test keeps failing, so mark this as expected failure for now. - @unittest.expectedFailure def test_registerUnboundInstanceMethod_raisesException(self): unboundInstMethod = ExampleClass.method with self.assertRaises(TypeError): @@ -450,6 +464,22 @@ def test_lambdaHandler(self): self.action.notify(a='a value') self.assertEqual(calledKwargs, {'a': 'a value'}) + def test_partialHandler(self): + """ Test that a L{functools.partial} can be used as a handler. + Note: the partial must be kept alive, since register uses a weak reference to it. + """ + + calledKwargs = {} + + def handler(a, b): + calledKwargs['a'] = a + calledKwargs['b'] = b + + p = partial(handler, a=1) + self.action.register(p) + self.action.notify(b='a value') + self.assertEqual(calledKwargs, {'a': 1, 'b': 'a value'}) + def test_handlerException(self): """Test that a handler which raises an exception doesn't affect later handlers. """ @@ -496,6 +526,17 @@ def handler(a=0): self.action.notify(a=1) self.assertEqual(calledKwargs, {"a": 1}) + def test_handlerParamsWithRequiredKwarg(self): + """ Test that a handler that accepts required keyword arguments receives arguments + """ + calledKwargs = {} + def handler(*, a): + calledKwargs["a"] = a + + self.action.register(handler) + self.action.notify(a=1) + self.assertEqual(calledKwargs, {"a": 1}) + class TestFilter(unittest.TestCase): def setUp(self): @@ -602,6 +643,19 @@ def handler(value, a=0): self.filter.apply("some value", a=1) self.assertEqual(calledKwargs, {"value": "some value", "a": 1}) + def test_handlerParamsWithRequiredKwarg(self): + """ Test that a handler that accepts required keyword arguments receives arguments + """ + calledKwargs = {} + + def handler(value, *, a): + calledKwargs['value'] = value + calledKwargs["a"] = a + + self.filter.register(handler) + self.filter.apply("some value", a=1) + self.assertEqual(calledKwargs, {"value": "some value", "a": 1}) + class TestDecider(unittest.TestCase): def setUp(self): @@ -714,4 +768,16 @@ def handler(a=0): self.decider.register(handler) self.decider.decide(a=1) - self.assertEqual(calledKwargs, {"a": 1}) \ No newline at end of file + self.assertEqual(calledKwargs, {"a": 1}) + + def test_handlerParamsWithRequiredKwarg(self): + """ Test that a handler that accepts required keyword arguments receives arguments + """ + calledKwargs = {} + + def handler(*, a): + calledKwargs["a"] = a + + self.decider.register(handler) + self.decider.decide(a=1) + self.assertEqual(calledKwargs, {"a": 1})