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

Add support for the ruby_package file option added in protobuf v3.6.0 #20

Closed
wants to merge 2 commits into from

Conversation

lwc
Copy link

@lwc lwc commented Aug 28, 2018

Admittedly this is a little hacky, but once descriptor.proto in golang/protobuf catches up to the protocolbuffers/protobuf version the hack can come out.

Fixes #19

func fileToRubyModules(file *descriptor.FileDescriptorProto) []string {

r := new(RubyPackageParser)
proto.Unmarshal(file.Options.XXX_unrecognized, r)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the way to check if the option is recognized or not?

@@ -273,3 +281,12 @@ func isASCIILower(c byte) bool {
func isASCIIDigit(c byte) bool {
return '0' <= c && c <= '9'
}

// RubyPackageParser can parse the ruby package option added in protobuf v3.6.0 but not yet in golang/protobuf
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not yet in golang/protobuf

this code should not concern with the version of golang/protobuf or protocolbuffers/protobuf that is being used, because the version may change at anytime (and the comment would be worthless). Maybe a better way to describe this struct is that the Package property will contain the ruby_package option if the protobuf compiler supports it. I would just comment the field like this:

// RubyPackageParser struct to use with proto.Unmarshal and the file descriptor. 
type RubyPackageParser struct {
	// RubyPackageOption is the ruby_package option in the proto file.
	// It is empty if there is no option, or if not supported by the proto package
	// This option was added in protocolbuffers/protobuf v3.6.0+
	OptionRubyPackage string `protobuf:"bytes,45,opt,name=ruby_package,json=rubyPackage,proto2" json:"ruby_package,omitempty"`
}

@marioizquierdo
Copy link
Collaborator

marioizquierdo commented Sep 16, 2018

The generator code has this imports:

import (
	// ...
	"github.com/golang/protobuf/proto"
	"github.com/golang/protobuf/protoc-gen-go/descriptor"
	plugin "github.com/golang/protobuf/protoc-gen-go/plugin"
)

I think this means that it always generates code with golang/protobuf, instead of with protocolbuffers/protobuf, is that right?

However, when generating code with the protoc command, if the option ruby_package is added, the service.pb.rb file properly contains the modules specified in the option. I think this means that golang/protobuf already supports the ruby_package option?

I installed protoc through brew, that uses the source https://github.com/google/protobuf/ and shows version libprotoc 3.6.0.

@marioizquierdo
Copy link
Collaborator

marioizquierdo commented Oct 9, 2018

@lwc any chance you can take a look at this?

@marioizquierdo
Copy link
Collaborator

Closing for inactivity

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.

2 participants