Navigation Menu

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(build): Reduce Default bound requirement #974

Merged

Conversation

conradludgate
Copy link
Contributor

Motivation

#973

It seems the Default bound was overly restrictive

Solution

Move the Default bound to the with_interceptor function instead of the whole impl

@LucioFranco
Copy link
Member

I think in something like this an overly expressive bound is good to help guide the compiler to better errors. Is there something that this is blocking for you?

@conradludgate
Copy link
Contributor Author

Yeah, as mentioned in the other issue, it blocks a lot of service layers that would otherwise be valid. I see your point however.

There was a fix that I can make to box the responses or something on my end, but it seemed better to me if it wasn't necessary in the first place, as this is more churn for our company wide jump to tonic 0.7

If you don't want this, then I'd advise this limitation and ways to get around it be documented somewhere that's easy to find

@LucioFranco
Copy link
Member

Oh I see now in #973 :)

@davidpdrsn davidpdrsn changed the title fix: grpcservice responsebody does not need default fix: move Default bound in generated code to only where its needed Apr 19, 2022
@davidpdrsn
Copy link
Member

Outside of the one comment I had I think this looks good!

Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
@LucioFranco
Copy link
Member

Ok looks like we need to fix the one CI failure and then we can merge. I don't think this is a breaking change so we can get this out in a patch release.

@LucioFranco LucioFranco changed the title fix: move Default bound in generated code to only where its needed fix(build): Reduce Default bound requirement Apr 20, 2022
@LucioFranco LucioFranco merged commit 4533a6e into hyperium:master Apr 20, 2022
@conradludgate conradludgate deleted the fix-tonic-response-body-default branch April 20, 2022 20:33
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.

None yet

3 participants