Skip to content

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

Merged
feerrenrut merged 7 commits into
nvaccess:threshold_py3_stagingfrom
LeonarddeR:py3_extensionPoints
Jun 28, 2019

Conversation

@LeonarddeR

Copy link
Copy Markdown
Collaborator

Link to issue number:

Fixes #9770

Summary of the issue:

Using callWithSupportedKwargs to call a function results in a deprecation warning about inspect.getargspec

Description of how this pull request fixes the issue:

This pr contains a major rewrite of callWithSUpportedKwargs. In python 2, we had to process positional arguments ourselves, mapping positional to keyword args and vice versa.
Python 3 contains inspect.Signature, a very handy class that for fills this need. This means that in callWithSUpportedKwargs, the only processing we have to do ourselves is not pass unsupported kwargs to the handler, but only if there is no **kwargs parameter in the handler. All other logic is handled by the inspect module.

Testing performed:

Unit tests. I also added some new ones for:

  1. Required kwargs: def hello(*, requiredKwarg):
  2. partials

Known issues with pull request:

None

Change log entry:

  • Changes for developers
    • extensionPoints classes and the callWithSupportedKwargs function now support functools.partial instances and functions with required keyword arguments.

@LeonarddeR
LeonarddeR requested a review from feerrenrut June 20, 2019 06:10

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please investigate the failing test? Does it still fail? Search for @unittest.expectedFailure

This looks like a much simpler implementation. Hopefully the unit test coverage is good enough to catch all the behaviour we care about. The tests all pass right?

Could you also check that the doc strings are up to date for the testcases you modified.

Comment thread tests/unit/test_extensionPoints.py Outdated
def test_instanceMethodHandlerTakesParams_givenPositional(self):
"""Test to ensure that a instance method handler gets the correct arguments, including implicit "self"
Handler takes a parameter.
Handler takes a positional parameter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this confuses things, I agree that the terminology seems to be quite inconsistent. I tend to think about the parameters from two different perspectives. The function definition perspective, and the caller perspective.

  • From the function definition, in this case handlerMethod has one parameter, it has no default value, the function does not take a variable number of positional arguments, or a varliable number of keyword arguments.
  • From the caller perspective, handlerMethod is called with a single positional argument in this case. It would also be acceptable to call it with a single keyword argument a="a value"

I think there is often confusion on the function definition side, resulting in a parameter with a default value being thought of as a keyword argument, when it can actually be called using a positional argument, a keyword argument, or be missing entirely.

@LeonarddeR
LeonarddeR requested a review from feerrenrut June 21, 2019 06:50
@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@feerrenrut: I believe I've addressed your comments, but please point me at it if you think otherwise. I also removed the expected failure and improved the logic for register to catch bunbound methods properly. I guess it is safe to still call them unbound methods, I don't know how they are called in Python 3 idiom actually.

@feerrenrut
feerrenrut merged commit ec1c253 into nvaccess:threshold_py3_staging Jun 28, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Jun 28, 2019
@LeonarddeR
LeonarddeR deleted the py3_extensionPoints branch August 23, 2025 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants