Python 3: work around deprecation warning with a major rewrite of extensionPoints.callWithSUpportedKwargs, and add some tests for Py3 specific functionality

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)
+	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)

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})
+		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})