Skip to content

fix: remove duplicated keyword argument timeout #1870

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 21, 2023
Merged

fix: remove duplicated keyword argument timeout #1870

merged 6 commits into from
Apr 21, 2023

Conversation

m9810223
Copy link
Contributor

Traceback:

    await page.locator('#...').screenshot(
  File "/usr/lib/python3.8/site-packages/playwright/async_api/_generated.py", line 16317, in screenshot
    await self._impl_obj.screenshot(
  File "/usr/lib/python3.8/site-packages/playwright/_impl/_locator.py", line 479, in screenshot
    return await self._with_element(
  File "/usr/lib/python3.8/site-packages/playwright/_impl/_locator.py", line 105, in _with_element
    return await task(
  File "/usr/lib/python3.8/site-packages/playwright/_impl/_locator.py", line 480, in <lambda>
    lambda h, timeout: h.screenshot(timeout=timeout, **params)
TypeError: screenshot() got multiple values for keyword argument 'timeout'

I fix this problem with the coding style:

params = locals_to_params(locals())
del params["target"]
return await self._frame.drag_and_drop(
self._selector, target._selector, strict=True, **params
)

m9810223 and others added 3 commits April 15, 2023 00:18
```shell
RuntimeWarning: coroutine 'PlaywrightContextManager.__aexit__' was never awaited
playwright.stop()
```
TypeError: screenshot() got multiple values for keyword argument 'timeout'
@@ -517,6 +517,7 @@ async def screenshot(
mask: List["Locator"] = None,
) -> bytes:
params = locals_to_params(locals())
del params["timeout"]
return await self._with_element(
lambda h, timeout: h.screenshot(timeout=timeout, **params)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lambda h, timeout: h.screenshot(timeout=timeout, **params)
lambda h, timeout: h.screenshot(**params)

do you mind adding a test for it? I think also this is a cleaner bugfix.

@m9810223
Copy link
Contributor Author

m9810223 commented Apr 21, 2023

Hi @mxschmitt,

Sure. I can add the test.

But I have one question about your solution:

  • The lambda function also has an arg: timeout. If you remove timeout=timeout that means timeout arg in the lambda function is uesless.
    So which (variable) value these functions should revieve?
    async def select_text(self, force: bool = None, timeout: float = None) -> None:
        #                                           [m-arg]
        params = locals_to_params(locals())
        del params["timeout"] # [PR]
        return await self._with_element(
            lambda h, timeout: h.select_text(timeout=timeout, **params), timeout
        #             [f-arg]                        [f-arg?]    [PR]    [m-arg?]
        )

    # m-arg -> method arg
    # f-arg -> lambda function arg

@mxschmitt
Copy link
Member

The lambda function also has an arg: timeout. If you remove timeout=timeout that means timeout arg in the lambda function is useless.

You are right, I didn't realise that. It should receive of course the lambda timeout.

I think my favourite fix would be: h.select_text(**{**params, "timeout": timeout}), which makes it clear that we have a timeout override instead of a random property deletion.

@m9810223
Copy link
Contributor Author

Cool! I think dict unpacking style is better than using the del syntax, too.

@m9810223
Copy link
Contributor Author

m9810223 commented Apr 21, 2023

  • **{**params} cannot pass mypy test:

    mypy.....................................................................Failed
    - hook id: mypy
    - exit code: 1
    
    playwright/_impl/_locator.py:521: error: Argument 1 to "screenshot" of "ElementHandle" has incompatible type "**Dict[str, float]"; expected "Optional[Literal['jpeg', 'png']]"
    playwright/_impl/_locator.py:521: error: Argument 1 to "screenshot" of "ElementHandle" has incompatible type "**Dict[str, float]"; expected "Union[str, Path, None]"
    playwright/_impl/_locator.py:521: error: Argument 1 to "screenshot" of "ElementHandle" has incompatible type "**Dict[str, float]"; expected "Optional[int]"
    playwright/_impl/_locator.py:521: error: Argument 1 to "screenshot" of "ElementHandle" has incompatible type "**Dict[str, float]"; expected "Optional[bool]"
    playwright/_impl/_locator.py:521: error: Argument 1 to "screenshot" of "ElementHandle" has incompatible type "**Dict[str, float]"; expected "Optional[Literal['allow', 'disabled']]"
    playwright/_impl/_locator.py:521: error: Argument 1 to "screenshot" of "ElementHandle" has incompatible type "**Dict[str, float]"; expected "Optional[Literal['hide', 'initial']]"
    playwright/_impl/_locator.py:521: error: Argument 1 to "screenshot" of "ElementHandle" has incompatible type "**Dict[str, float]"; expected "Optional[Literal['css', 'device']]"
    playwright/_impl/_locator.py:521: error: Argument 1 to "screenshot" of "ElementHandle" has incompatible type "**Dict[str, float]"; expected "Optional[List[Locator]]"
    playwright/_impl/_locator.py:553: error: Argument 1 to "select_text" of "ElementHandle" has incompatible type "**Dict[str, float]"; expected "Optional[bool]"
    Found 9 errors in 1 file (checked 1 source file)
    
  • **dict(**params) does not support overriding

  • I think discard locals_to_params(locals()) and pass every args is easier to fit type checking and test.

@mxschmitt mxschmitt merged commit f4a68cd into microsoft:main Apr 21, 2023
@mxschmitt
Copy link
Member

Thanks!

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.

2 participants