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

protoc-gen-twirp_ruby: obey ruby_package #35

Merged
merged 2 commits into from
Jun 3, 2019

Conversation

vmg
Copy link
Contributor

@vmg vmg commented Apr 29, 2019

The ProtocolBuffer FileDescriptorProto was updated with support for custom Ruby packages last year. This means that if you have a .proto file with a ruby_package = "foo option set and run it through protoc-gen-twirp_ruby, the resulting package_pb.rb will contain a different module name than package_twirp.rb.

Fixing this is easy enough though!

@vmg
Copy link
Contributor Author

vmg commented Apr 30, 2019

OK, I lied, fixing this wasn't easy enough! In order to properly support ruby_package for all type dependencies in the generator, we need to depend on the typemap package from twitchtv/twirp. Unfortunately, that package is in an internal folder, so we need to vendor it directly on this repository to access it.

With access to the typemap package, the implementation in main.go is quite straightforward. I've added a Gopkg.toml to ensure we're vendoring the right version of the protobuf package.

I'd love to discuss what's the ideal way to map this dependency on the typemap package. Should I move it out of twitchtv/twirp's internal folder and depend on that repository? Or is it OK to just keep a copy vendored here?

@gorzell
Copy link

gorzell commented May 20, 2019

@spenczar is there anything blocking this from being merged?

@spenczar
Copy link
Contributor

@marioizquierdo @cyrusaf ^

@cyrusaf
Copy link
Contributor

cyrusaf commented May 20, 2019

Thanks for the ping -- I'm taking a look now. @spenczar I'm wondering if it makes sense to share the internal/ packages between the Go and Ruby twirp generators? This PR copies the typemap package over and when I originally wrote the Ruby Twirp generator I copied over other code from theinternal/ folder of the Go generator. What are your thoughts?

@spenczar
Copy link
Contributor

@cyrusaf, https://github.com/twitchtv/protogen pulls those internal packages out into a repo. It was done to support other generators, but it isn't used by either the Go or Ruby generators; I think it's kind of a failed experiment.

Moving to it seems nice, but it makes versioning much more complicated. I think using the copy you have here is probably preferable, even if it diverges, since the divergences will be pretty specific to this project. Maybe we can discuss that more in a different issue, if you think it's worth thinking more about.

@vmg
Copy link
Contributor Author

vmg commented Jun 3, 2019

@cyrusaf: Sorry for the delay! I've fixed the test suite. It was a bit more involved than I expected because I had to add fixtures.

@cyrusaf
Copy link
Contributor

cyrusaf commented Jun 3, 2019

Thanks for making this change -- it looks great. This PR made me realize that we should have end to end tests for generating the client/server and driving the existing unit tests through them. I've created #37 to track this.

@cyrusaf cyrusaf merged commit 025101e into arthurnn:master Jun 3, 2019
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.

4 participants