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

[transport/http] adding request and response encoders for Protobuf #809

Merged
merged 4 commits into from
Dec 19, 2018

Conversation

jprobinson
Copy link
Contributor

Bringing these over from our marvin framework now that we can use Gizmo + go-kit in Google App Engine (hooray!)

I'm not 100% sure what we should do in the case of error while marshalling the protobuf so just commenting with a frowny face for now. Any thoughts for master...jprobinson:encode-proto#diff-05760750cf2e3aac59bd773896ee182fR202?

This resolves #808.

@jprobinson
Copy link
Contributor Author

I've pushed updates with the new functions in a new subpackage: transport/http/proto

@jprobinson
Copy link
Contributor Author

jprobinson commented Dec 10, 2018

I've been thinking about the package name a bit and proto may be a bit annoying as it matches the main proto package. Any thoughts on naming it pb or httppb?

@peterbourgon
Copy link
Member

peterbourgon commented Dec 10, 2018

Go kit consumers are already essentially required to make use of import aliases to get e.g. transport/http into their lexical scope. I think for now it's best to leverage that assumption, keep with the naming conventions, and stick with proto. (When we get around to a repo and package restructuring, I'll probably stick kit as a prefix to most package names.)

@peterbourgon
Copy link
Member

Cool, I like this well enough. Any objections? I'll merge in a couple days otherwise.

@jprobinson
Copy link
Contributor Author

I don't mean to be a bugger, but any chance we can merge this one soon-ish? I'm working on some internal docs/boilerplates and would love to include this lil tidbit.

@peterbourgon
Copy link
Member

Oh! I thought I'd done it. Mea culpa.

@peterbourgon peterbourgon merged commit d8776e0 into go-kit:master Dec 19, 2018
@jprobinson jprobinson deleted the encode-proto branch December 19, 2018 21:24
@jprobinson
Copy link
Contributor Author

Thanks, Peter!

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.

Add support for encoding Protobuf requests and responses to transport/http
4 participants