Repository navigation
Run interceptor and compression tests once against sync and async clients #387
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
Changes from 5 commits
6980d53
f7623f4
6e7d3c2
e933a38
c19ad02
983e285
d2cfce5
9a8dee8
0a1057b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,14 +1,35 @@ | ||||||
| from __future__ import annotations | ||||||
|
|
||||||
| from typing import TYPE_CHECKING | ||||||
| import asyncio | ||||||
| from collections.abc import AsyncIterator, Iterator | ||||||
| from typing import TYPE_CHECKING, Any, Literal | ||||||
|
|
||||||
| from pyqwest import Client, SyncClient | ||||||
| from pyqwest.testing import ASGITransport, WSGITransport | ||||||
|
|
||||||
| from connectrpc._compression import IdentityCompression | ||||||
| from connectrpc.compression.brotli import BrotliCompression | ||||||
| from connectrpc.compression.gzip import GzipCompression | ||||||
| from connectrpc.compression.zstd import ZstdCompression | ||||||
|
|
||||||
| from .connectrpc.example.haberdasher_connect import ( | ||||||
| Haberdasher, | ||||||
| HaberdasherASGIApplication, | ||||||
| HaberdasherClient, | ||||||
| HaberdasherClientSync, | ||||||
| HaberdasherSync, | ||||||
| HaberdasherWSGIApplication, | ||||||
| ) | ||||||
|
|
||||||
| if TYPE_CHECKING: | ||||||
| from collections.abc import Callable, Mapping | ||||||
|
|
||||||
| from pyqwest import SyncTransport, Transport | ||||||
|
|
||||||
| from connectrpc.compression import Compression | ||||||
| from connectrpc.request import RequestContext | ||||||
|
|
||||||
| from .connectrpc.example.haberdasher_pb import Hat, Size | ||||||
|
|
||||||
|
|
||||||
| def resolve_compression(encoding: str) -> Compression: | ||||||
|
|
@@ -24,3 +45,75 @@ def resolve_compression(encoding: str) -> Compression: | |||||
| case _: | ||||||
| msg = f"unknown encoding '{encoding}'" | ||||||
| raise ValueError(msg) | ||||||
|
|
||||||
|
|
||||||
| def haberdasher_client(transport: Transport, **kwargs) -> HaberdasherClient: | ||||||
| return HaberdasherClient( | ||||||
| "http://localhost", http_client=Client(transport), **kwargs | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| def haberdasher_client_sync( | ||||||
| transport: SyncTransport, **kwargs | ||||||
| ) -> HaberdasherClientSync: | ||||||
| return HaberdasherClientSync( | ||||||
| "http://localhost", http_client=SyncClient(transport), **kwargs | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| async def call( | ||||||
| client: HaberdasherClient | HaberdasherClientSync, | ||||||
| method: str, | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be an enum. Alternatively, arguably less type-safe but easier for the callers, I think we can accept the method itself, and
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in d2cfce5: |
||||||
| request: Size | list[Size], | ||||||
| **kwargs, | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method seems more useful than above while still having the kwargs problem. FWIW I avoid this sort of helper in all my repos for a preference to have IDE-supported / well typed code over reducing boilerplate. I wouldn't block this though
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
| ) -> Hat | list[Hat]: | ||||||
| """Calls method on client, passing and returning streams as lists. | ||||||
|
|
||||||
| A sync client runs in a worker thread, as it would in a sync program. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed in d2cfce5. |
||||||
| """ | ||||||
| if isinstance(client, HaberdasherClientSync): | ||||||
|
|
||||||
| def run() -> Hat | list[Hat]: | ||||||
| req = iter(request) if isinstance(request, list) else request | ||||||
| result = getattr(client, method)(req, **kwargs) | ||||||
| return list(result) if isinstance(result, Iterator) else result | ||||||
|
|
||||||
| return await asyncio.to_thread(run) | ||||||
|
|
||||||
| async def stream(requests: list[Size]) -> AsyncIterator[Size]: | ||||||
| for r in requests: | ||||||
| yield r | ||||||
|
|
||||||
| result = getattr(client, method)( | ||||||
| stream(request) if isinstance(request, list) else request, **kwargs | ||||||
| ) | ||||||
| if isinstance(result, AsyncIterator): | ||||||
| return [r async for r in result] | ||||||
| return await result | ||||||
|
|
||||||
|
|
||||||
| def unary_client( | ||||||
| mode: Literal["async", "sync"], | ||||||
| make_hat: Callable[[Size, RequestContext], Hat], | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There doesn't seem to be a good reason to accept make_hat over haberdashers, they are minimal boilerplate. mode would go away. But this method has a ergonomic issue of having two untyped kwargs type of parameters ;) I don't know if it's worth having this
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed it wasn't worth it; removed in 983e285, which restores the tests it merged. |
||||||
| app_options: Mapping[str, Any] | None = None, | ||||||
| **kwargs, | ||||||
| ) -> HaberdasherClient | HaberdasherClientSync: | ||||||
| """Returns a client of an ASGI or WSGI server whose MakeHat calls make_hat. | ||||||
|
|
||||||
| app_options are passed to the application and kwargs to the client. | ||||||
| """ | ||||||
| if mode == "async": | ||||||
|
|
||||||
| class UnaryHaberdasher(Haberdasher): | ||||||
| async def make_hat(self, request, ctx): | ||||||
| return make_hat(request, ctx) | ||||||
|
|
||||||
| app = HaberdasherASGIApplication(UnaryHaberdasher(), **(app_options or {})) | ||||||
| return haberdasher_client(ASGITransport(app), **kwargs) | ||||||
|
|
||||||
| class UnaryHaberdasherSync(HaberdasherSync): | ||||||
| def make_hat(self, request, ctx): | ||||||
| return make_hat(request, ctx) | ||||||
|
|
||||||
| app = HaberdasherWSGIApplication(UnaryHaberdasherSync(), **(app_options or {})) | ||||||
| return haberdasher_client_sync(WSGITransport(app), **kwargs) | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout the file, the lack of type safety (more importantly IDE completion) on
kwargsis a regression I think. IIRC Python unfortunately doesn't have a pattern for typing delegate method kwargs so if we can't, I think it's worth the small extra boilerplate at the callers to have the IDE supportThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, dropped both helpers in 9a8dee8; tests construct clients directly again.