chore(types): improve typing #789

kumaraditya303 · 2021-07-03T11:05:33Z

Fixes #788

kumaraditya303 · 2021-07-03T11:06:18Z

playwright/_impl/_async_base.py

@@ -68,17 +73,19 @@ def _wrap_handler(self, handler: Any) -> Callable[..., None]:
            return mapping.wrap_handler(handler)
        return handler

-    def on(self, event: str, f: Any) -> None:
+    def on(self, event: str, f: Callable[..., Union[Awaitable[None], None]]) -> None:


This will accept both sync and async event listeners

Let's only do one thing at a time in each PR. If this PR is about cast, it should only touch cast.

Separated it to #790

kumaraditya303 · 2021-07-03T11:07:07Z

playwright/_impl/_sync_base.py

@@ -110,17 +115,17 @@ def _wrap_handler(self, handler: Any) -> Callable[..., None]:
            return mapping.wrap_handler(handler)
        return handler

-    def on(self, event: str, f: Any) -> None:
+    def on(self, event: str, f: Callable[..., None]) -> None:


Accepts a Callable

kumaraditya303 · 2021-07-03T11:08:20Z

playwright/_impl/_connection.py

@@ -319,11 +319,12 @@ def _replace_guids_with_channels(self, payload: Any) -> Any:
        return payload


-def from_channel(channel: Channel) -> Any:
+def from_channel(channel: Channel) -> ChannelOwner:


This works currently but ChannelOwner is better than Any

You don't need this change to move to cast. It does not hurt, but this comment is misleading

Updated comment

pavelfeldman · 2021-07-05T05:18:29Z

playwright/_impl/_artifact.py

@@ -37,7 +37,7 @@ async def path_after_finished(self) -> Optional[pathlib.Path]:
        return pathlib.Path(await self._channel.send("pathAfterFinished"))

    async def save_as(self, path: Union[str, Path]) -> None:
-        stream = cast(Stream, from_channel(await self._channel.send("saveAsStream")))
+        stream: Stream = from_channel(await self._channel.send("saveAsStream"))


I just tried following locally and mypy was happy...

stream: Stream = self

Stream inherits from ChannelOwner and from_channel returns ChannelOwner hence it is happy

pavelfeldman · 2021-07-05T05:18:51Z

playwright/_impl/_async_base.py


 from playwright._impl._impl_to_api_mapping import ImplToApiMapping, ImplWrapper

 mapping = ImplToApiMapping()


 T = TypeVar("T")
-Self = TypeVar("Self", bound="AsyncBase")
+Self = TypeVar("Self", bound="AsyncContextManager")


Let's only deal with cast here.

pavelfeldman · 2021-07-05T05:18:55Z

playwright/_impl/_async_base.py



 class AsyncEventInfo(Generic[T]):
-    def __init__(self, future: asyncio.Future) -> None:
+    def __init__(self, future: "asyncio.Future[T]") -> None:


Let's only deal with cast here.

pavelfeldman · 2021-07-05T05:18:59Z

playwright/_impl/_async_base.py

@@ -39,13 +39,18 @@ def is_done(self) -> bool:


 class AsyncEventContextManager(Generic[T]):
-    def __init__(self, future: asyncio.Future) -> None:
-        self._event: AsyncEventInfo = AsyncEventInfo(future)
+    def __init__(self, future: "asyncio.Future[T]") -> None:


Let's only deal with cast here.

pavelfeldman · 2021-07-05T05:19:11Z

playwright/_impl/_async_base.py

        """Registers the function ``f`` to the event name ``event``."""
        self._impl_obj.on(event, self._wrap_handler(f))

-    def once(self, event: str, f: Any) -> None:
+    def once(self, event: str, f: Callable[..., Union[Awaitable[None], None]]) -> None:


pavelfeldman · 2021-07-05T05:19:15Z

playwright/_impl/_async_base.py

        """The same as ``self.on``, except that the listener is automatically
        removed after being called.
        """
        self._impl_obj.once(event, self._wrap_handler(f))

-    def remove_listener(self, event: str, f: Any) -> None:
+    def remove_listener(
+        self, event: str, f: Callable[..., Union[Awaitable[None], None]]


pavelfeldman · 2021-07-05T05:19:50Z

playwright/_impl/_connection.py

@@ -319,11 +319,12 @@ def _replace_guids_with_channels(self, payload: Any) -> Any:
        return payload


-def from_channel(channel: Channel) -> Any:
+def from_channel(channel: Channel) -> ChannelOwner:


You don't need this change to move to cast. It does not hurt, but this comment is misleading

pavelfeldman · 2021-07-05T05:20:13Z

playwright/_impl/_sync_base.py

@@ -167,4 +172,7 @@ def __exit__(
        exc_val: BaseException,
        traceback: TracebackType,
    ) -> None:
-        self.close()  # type: ignore
+        self.close()


One thing at a time

pavelfeldman · 2021-07-05T23:14:46Z

Please open it when it is ready for review, otherwise it is unclear whether you updated the patch or just replied to comments.

kumaraditya303 · 2021-07-06T02:46:58Z

I has separated the async base changes to another PR and waiting here for it to get merged before rebasing. Here no change is required just a rebasing

kumaraditya303 commented Jul 3, 2021

View reviewed changes

kumaraditya303 marked this pull request as draft July 3, 2021 11:08

chore(types): improve typing

8d2e112

kumaraditya303 marked this pull request as ready for review July 3, 2021 11:59

pavelfeldman reviewed Jul 5, 2021

View reviewed changes

pavelfeldman closed this Jul 5, 2021

chore(types): improve typing #789

chore(types): improve typing #789

Uh oh!

Conversation

kumaraditya303 commented Jul 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumaraditya303 Jul 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavelfeldman commented Jul 5, 2021

Uh oh!

kumaraditya303 commented Jul 6, 2021

Uh oh!

Uh oh!

kumaraditya303 Jul 3, 2021 •

edited

Loading