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

fix(codec): Remove Default bound on Codec #894

Merged
merged 1 commit into from
Jan 24, 2022
Merged

fix(codec): Remove Default bound on Codec #894

merged 1 commit into from
Jan 24, 2022

Conversation

andrewhickman
Copy link
Contributor

Motivation

I implemented a custom codec to use with a dynamic method where the types are not known at compile time. However this bound is mildly annoying since there's no sensible way to implement default
https://github.com/andrewhickman/grpc-client/blob/487c5409e78afef200a064c81ae1ade1b37c3c28/src/grpc/codec.rs#L63

Solution

The bound is never used so we can just remove it. I think this is technically a breaking change for consumers of the Codec trait, however anything using ProstCodec, like generated code, still works.

This bound was never used, and prevents implementing `Codec` for dynamic
message types, e.g. https://github.com/andrewhickman/grpc-client/blob/master/src/grpc/codec.rs.
@LucioFranco
Copy link
Member

@davidpdrsn I forget did you remove the need for default when you changed some of the compression stuff?

@davidpdrsn
Copy link
Member

Hm no I don't think so.

I tried bisecting it but had to go so far back that I hit

~/de/ma/tonic [HEAD @ 6dbe88d445e37 * bisect]
❯ cargo c --all-features --tests -p tonic
    Updating crates.io index
    Updating git repository `https://github.com/tower-rs/tower`
error: no matching package named `tower-balance` found
location searched: https://github.com/tower-rs/tower
required by package `tonic v0.1.0-alpha.6`

Seems fine to me to the Default requirement.

@LucioFranco LucioFranco changed the title Remove Default bound on Codec fix(codec): Remove Default bound on Codec Jan 24, 2022
@LucioFranco LucioFranco merged commit d574cfd into hyperium:master Jan 24, 2022
@andrewhickman andrewhickman deleted the remove-default-bound branch January 26, 2022 00:05
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