Skip to content
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

[Feature] rename TimeoutError or make it extend the builtin TimeoutError #1495

Open
DetachHead opened this issue Aug 15, 2022 · 8 comments
Open

Comments

@DetachHead
Copy link

TimeoutError shadows a builtin exception with the same name, which leads to confusion

try:
    page.click("foo")
except TimeoutError: # builtins.TimeoutError
    # this doesn't run because it raises a playwright.sync_api.TimeoutError
    print("failed to click element")
@DetachHead DetachHead changed the title [Feature] rename `TimeoutError [Feature] rename TimeoutError Aug 15, 2022
@KotlinIsland
Copy link

This can be super sus if the import (from playwright.sync_api import TimeoutError) gets removed, your code will silently change behavior with no warning.

@rwoll
Copy link
Member

rwoll commented Aug 15, 2022

You can rename it within your own project like: from playwright.sync_api import sync_playwright, TimeoutError as PlaywrightTimeoutError.

While this doesn't address the shadowing and omission concern, Playwright cannot break its API by renaming this completely.

Perhaps doing:

from playwright.sync_api import sync_playwright, TimeoutError
class PlaywrightTimeoutError(TimeoutError):
   pass

would be a backwards compatible compromise. (The examples and your code could then be changed to look for PlaywrightTimeoutError.) Would this work for you? If so, we could discuss it further?

@rwoll rwoll added the triaging label Aug 15, 2022
@DetachHead
Copy link
Author

perhaps simply making playwright.sync_api.TimeoutError extend both Error and the builtin TimeoutError would solve the problem?

_BuiltinTimeoutError = TimeoutError


class TimeoutError(Error, _BuiltinTimeoutError):
    ...

that way there's no backward incompatible change and code that catches builtins.TimeoutError will still work

@KotlinIsland
Copy link

Probably want something like this:

class TimeoutError(Error, __builtins__.TimeoutError):
    pass

@DetachHead DetachHead changed the title [Feature] rename TimeoutError [Feature] rename TimeoutError or make it extend the builtin TimeoutError Aug 16, 2022
@mxschmitt

This comment was marked as outdated.

@mxschmitt mxschmitt reopened this Feb 20, 2024
@danphenderson
Copy link
Contributor

danphenderson commented Feb 21, 2024

@mxschmitt why was this reopened and the commit reverted?

@danphenderson
Copy link
Contributor

@mxschmitt, as a contributor who volunteered his time, may I please have an explanation? Without an explanation, I am highly unmotivated to continue contributing. Overall, I am just asking for more transparency.

@mxschmitt
Copy link
Member

See #2312 (comment) for the reason. Since there is a good workaround we think its ok to hold off for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants