Fix handler wrapping for keyword-only parameters#3084
Conversation
|
@microsoft-github-policy-service agree |
biswajeetdev
left a comment
There was a problem hiding this comment.
The fix correctly identifies the problem: inspect.signature(handler).parameters includes KEYWORD_ONLY and VAR_KEYWORD entries in its count, so a handler like def blur(self, *, timeout=None) was getting counted as having 2 parameters and receiving a spurious positional arg it could not accept.
The new logic — counting only POSITIONAL_ONLY and POSITIONAL_OR_KEYWORD params, and falling back to len(args) when *args is present — is exactly right.
Two small suggestions:
1. Add a test for the *args / has_varargs branch
The current test covers keyword-only params, but the has_varargs path is not exercised:
def test_wrap_handler_passes_all_args_for_varargs_handler() -> None:
calls = []
def handler(*args):
calls.append(args)
ImplToApiMapping().wrap_handler(handler)("a", "b", "c")
assert len(calls[0]) == 32. POSITIONAL_ONLY params in practice
Python's POSITIONAL_ONLY kind (params before /) is rare in pure-Python code but common in C-extension signatures and some inspect-wrapped callables. The fix correctly includes them — just noting it is a good edge case to keep in mind if future test fixtures include C-extension handlers.
|
Addressed the review suggestion by adding coverage for the Verified with:
|
Fixes #3067
Summary
This fixes handler wrapping so keyword-only parameters are not counted as positional handler arguments.
Why
Before this change, a handler like
locator.blurwas treated as if it accepted the triggering locator because it has a keyword-onlytimeoutoption. Playwright then passed the locator positionally and crashed withTypeError.Test plan
python3 -m pytest tests/common/test_impl_to_api_mapping.pypython3 -m black --check playwright/_impl/_impl_to_api_mapping.py tests/common/test_impl_to_api_mapping.pypython3 -m mypy playwright/_impl/_impl_to_api_mapping.py