Skip to content

Conversation

@noel-yap
Copy link
Contributor

@noel-yap noel-yap commented Sep 5, 2017

Custom protoc options (https://developers.google.com/protocol-buffers/docs/proto3#custom_options) are implemented via extensions (https://developers.google.com/protocol-buffers/docs/reference/java-generated#extension). If a custom option is to be processed by a protoc plugin, the plugin code must register the extension. For example (from the docs):

ExtensionRegistry registry = ExtensionRegistry.newInstance();
registry.add(Baz.fooExt);
Foo foo = Foo.parseFrom(input, registry);

@salesforce-cla
Copy link

salesforce-cla bot commented Sep 5, 2017

Thanks for the contribution! Before we can merge this, we need @noel-yap to sign the Salesforce Contributor License Agreement.

@codecov
Copy link

codecov bot commented Sep 5, 2017

Codecov Report

Merging #27 into master will decrease coverage by 1.03%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #27      +/-   ##
============================================
- Coverage     58.14%   57.11%   -1.04%     
  Complexity       97       97              
============================================
  Files            19       19              
  Lines           442      450       +8     
  Branches         33       34       +1     
============================================
  Hits            257      257              
- Misses          177      185       +8     
  Partials          8        8
Impacted Files Coverage Δ Complexity Δ
...main/java/com/salesforce/jprotoc/ProtocPlugin.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ava/com/salesforce/jprotoc/jdk8/Jdk8Generator.java 0% <0%> (ø) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dd1eb8...75afb39. Read the comment docs.

@rmichela
Copy link
Collaborator

rmichela commented Sep 5, 2017

Thanks for the PR! Could you elaborate on what this does?

Copy link
Collaborator

@rmichela rmichela left a comment

Choose a reason for hiding this comment

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

Looking good! Can you also add a section to the JProtoc README.md with an example of how to author an generator that uses extensions?

generate(generators, Collections.emptyList());
}

public static void generate(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs JavaDoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Preconditions.checkArgument(!generators.isEmpty(), "generators.isEmpty()");
Preconditions.checkNotNull(extensions, "extensions");

ExtensionRegistry extensionRegistry = ExtensionRegistry.newInstance();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment explaining what the extensionRegistry does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

string name = 1;
}
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do I have to run the protobuf generator as part of the build process for my protobuf plugin? Do I need to add this to my plugin project's POM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not familiar with Maven but I don't see any reason anything in the build would have to change. All the processing is done at protoc run-time when protoc calls the protoc plugin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I'm not understanding correctly. Where does the type com.example.proto.options.ServerOptionsProto.server come from in the example below? Perhaps you could add an elaborated example in jprotoc-test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's generated by protoc. Those not creating custom options don't need to run protoc to generate the related Java code for the option. I'm assuming those creating custom options would know how to run protoc to do that. I can add a blurb about running protoc if you want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see...My JProtoc Protobuf generator project would itself contain Protobuf generated code (it's proto all the way down!). A line to say "Run the Protoc generator on the Extension proto" should suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@noel-yap
Copy link
Contributor Author

noel-yap commented Sep 6, 2017

I'm not understanding the output of the code coverage report. Some lines (eg ProtocPlugin.java:44) are marked red but I don't see how that could be since the entrypoint should have been covered by some previous test. Were the reports added later and there's actually no test covering ProtocPlugin.generate()? I'm not finding any test code that calls it.

@rmichela
Copy link
Collaborator

rmichela commented Sep 6, 2017

Ignore the CodeCov report. I'm still trying to get it working right. 😉

@rmichela rmichela merged commit b72d219 into salesforce:master Sep 6, 2017
rmichela pushed a commit that referenced this pull request Dec 18, 2022
…-exec-maven-plugin-3.1.0

build(deps): bump exec-maven-plugin from 1.6.0 to 3.1.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants