-
Notifications
You must be signed in to change notification settings - Fork 6
Make Compression protocol public and default to gzip-only #103
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
Conversation
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
stefanvanburen
left a comment
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.
👍, a nit and a question
src/connectrpc/compression/gzip.py
Outdated
| from . import Compression | ||
|
|
||
|
|
||
| class GZipCompression(Compression): |
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.
minorest of nits: I'd prefer Gzip to GZip here, to match the stdlib.
| interceptors: Iterable[Interceptor] = (), | ||
| read_max_bytes: int | None = None, | ||
| compressions: Iterable[str] | None = None, | ||
| compressions: Sequence[Compression] | None = None, |
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.
I might have just missed it reading through, but why do we need a Sequence instead of an Iterable here?
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.
Thanks for calling this out - in a different project I got feedback that Sequence could make sense for an ordered list, i.e. set doesn't work then. But on a second thought, I know that such a parameter would be Iterable in Java so what works there should work here and switched back to it. If a user passes a set anyways, they probably have a strong reason for it for better or worse
This reworks the interfaces related to compression to better align behavior with other connect implementations and provide more flexibility, and type safety
Compressionprotocol public. Can be used to implement custom compressionsCompressioninstead ofstrthroughout for type safetyCompressionimplementations for zstd and brotli are provided in public API to allow registering them, possibly with custom levels, as users need