Skip to content

support overloads and add overloads to wait_for_selector for when it returns None #851

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

Closed
wants to merge 8 commits into from

Conversation

DetachHead
Copy link

@DetachHead DetachHead commented Aug 19, 2021

  • optional arguments are now only automatically converted to keyword arguments if the method has no overloads, because otherwise it can invalidate overloads. it's expected that you explicitly specify what arguments should be keywords, like in the example below

  • overloads must be defined using @api_overload in order for the generate scripts to be able to see them at runtime.

    from playwright._impl._overload import api_overload
    
    @api_overload
    async def wait_for_selector(
        self,
        selector: str,
        *,
        timeout: float = None,
        state: Literal["attached", "visible"] = None,
        strict: bool = None,
    ) -> ElementHandle:
        pass
    
    @api_overload  # type: ignore[no-redef]
    async def wait_for_selector(
        self,
        selector: str,
        *,
        timeout: float = None,
        state: Literal["detached", "hidden"],
        strict: bool = None,
    ) -> None:
        pass
    
    async def wait_for_selector(  # type: ignore[no-redef]
        self,
        selector: str,
        *,
        timeout: float = None,
        state: Literal["attached", "detached", "hidden", "visible"] = None,
        strict: bool = None,
    ) -> Optional[ElementHandle]:
        ...

@ghost
Copy link

ghost commented Aug 19, 2021

CLA assistant check
All CLA requirements met.

@DetachHead DetachHead changed the title add overloads to wait_for_selector for when it returns None add overloads to wait_for_selector for when it returns None Aug 19, 2021
@mxschmitt
Copy link
Member

Hi! Thank you for your contribution! This needs to be added to the generated sync and async APIs, since we expose them and not the implementation classes. See e.g. in scripts/generate_async_api.py.

@DetachHead
Copy link
Author

DetachHead commented Aug 26, 2021

@mxschmitt the generate scripts don't work on overloads. when i run them they don't change anything because overloads aren't present in __dict__

when i implemented a workaround where i define the overloads with different names, another issue arises where it changes the location of the *. this makes the overloads invalid:

impl

    # this is my gross hacky way of writing overloads that the generate scripts can see. open to feedback on this approach
    async def _overload1_wait_for_selector(
        self,
        selector: str,
        timeout: float = None,
        state: Literal["attached", "visible"] = None,
        strict: bool = None,
    ) -> ElementHandle:
        ...

    async def _overload2_wait_for_selector(
            self,
            selector: str,
            timeout: float= None,
            *,
            state: Literal["detached", "hidden"],
            strict: bool = None,
    ) -> None:
        ...

    async def _overload3_wait_for_selector(
            self,
            selector: str,
            timeout: Optional[float],
            state: Literal["detached", "hidden"],
            strict: bool = None,
    ) -> None:
        ...

    async def wait_for_selector(
        self,
        selector: str,
        timeout: float = None,
        state: Literal["attached", "detached", "hidden", "visible"] = None,
        strict: bool = None,
    ) -> Optional[ElementHandle]:
        return await self._main_frame.wait_for_selector(**locals_to_params(locals()))

generated

    @typing.overload
    async def wait_for_selector(
        self,
        selector: str,
        *,
        state: Literal["attached", "visible"] = None,
        timeout: float = None
    ) -> "ElementHandle":
        pass

    @typing.overload
    async def wait_for_selector(
        self,
        selector: str,
        state: Literal["detached", "hidden"], # since `state` isn't optional in this overload, it gets moved to the wrong spot, making the overload invalid
        *,
        timeout: float = None
    ) -> NoneType:
        pass

    async def wait_for_selector(
        self,
        selector: str,
        *,
        state: Literal["attached", "detached", "hidden", "visible"] = None,
        timeout: float = None
    ) -> typing.Optional["ElementHandle"]:
        ...

any advice on how to get this working? and what exactly is the purpose of writing the method signatures differently to how they turn out in the generated ones? seems like a needless point of complexity in my opinion

@mxschmitt
Copy link
Member

There are a few reasons behind this extra layer, the biggest is that we add generated documentation via it from an external source and provide a sync and async wrapper around the implementation classes.

The generator would need to be adjusted to add support for overloads or manual insertions, we can also take a look at some point.

@DetachHead DetachHead changed the title add overloads to wait_for_selector for when it returns None support overloads and add overloads to wait_for_selector for when it returns None Aug 31, 2021
@DetachHead
Copy link
Author

DetachHead commented Aug 31, 2021

@mxschmitt i added support for overloads, edited the PR description with details

@mxschmitt
Copy link
Member

mxschmitt commented Sep 1, 2021

Hey @DetachHead, sorry for the delay. Will talk later with the team about it. We will likely end up adding two more methods to not have to rely on overloads (since we're adding in the future languages which don't support overloads). This gives us the same behaviour across all our languages (.NET, Java, etc.).

  • new: {Page,ElementHandle,Locator,Frame}.wait_for_selector_hidden(state=Literal["detached", "hidden"]...))
  • new: {Page,ElementHandle,Locator,Frame}.wait_for_selector_visible(state=Literal["visible", "attached"]...))

@mxschmitt
Copy link
Member

Closing since its stale, waiting also for user reports.

@mxschmitt mxschmitt closed this Feb 14, 2022
@DetachHead
Copy link
Author

Why close it?

Will talk later with the team about it. We will likely end up adding two more methods to not have to rely on overloads (since we're adding in the future languages which don't support overloads). This gives us the same behaviour across all our languages (.NET, Java, etc.).

did anything ever come of this?

@DetachHead
Copy link
Author

@mxschmitt typing.get_overloads is being introduced in python 3.11 https://docs.python.org/3.11/library/typing.html#typing.get_overloads

I can look at redoing this mr using it

@KotlinIsland
Copy link

@mxschmitt I think this is a good idea, can we see an update on this at all?

@KotlinIsland
Copy link

Looks like some hard coded overloads were supported in #897

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