-
Notifications
You must be signed in to change notification settings - Fork 7
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
Allowing only plugins to run, and allowing options for them #167
Allowing only plugins to run, and allowing options for them #167
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
==========================================
- Coverage 83.53% 83.28% -0.25%
==========================================
Files 26 26
Lines 850 855 +5
Branches 64 66 +2
==========================================
+ Hits 710 712 +2
- Misses 113 114 +1
- Partials 27 29 +2
|
@@ -547,6 +553,8 @@ public void execute() throws MojoExecutionException, MojoFailureException { | |||
.stream() | |||
.map(File::toPath) | |||
.collect(Collectors.toList())) | |||
.isPluginOnly(pluginOnly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this on the related issue but would setting Java support to be false in the existing configuration settings suffice for this feature, or is there another use case that you have in mind for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ascopes let me try that out. I believe I tried that at first and it wasn't working, but I could have something else as the issue at that time.
boolean pluginOnly; | ||
|
||
@Parameter | ||
@Nullable String pluginOption; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to add a JavaDoc to this field? Reason being it will be used to generate the documentation for this parameter in the generated GitHub pages. Worth adding a @since 1.3.0
here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we want to include metadata on plugins in a more controlled way whether it is worth revamping how plugins are defined so that you can tie options down to the specific plugins they apply to.
That'd possibly be an implication of a v2.0.0 release due to the breaking change (unless I keep the old ways of managing this around and just deprecate them) but I am more than happy to explore that possibility.
I did look into the idea of having just one way of specifying plugins that let you specify one of the path name, the URL, or the artifact... but I couldn't think of a good typesafe way of doing it. Nothing stopping me making another attempt though.
<plugins>
<plugin>
<type>PATH</type>
<name>protoc-gen-grpc-java</name>
<options>
<option>foobarbaz</option>
</options>
</plugin>
</plugins>
What are your thoughts on this? I might try to throw a proof of concept together this afternoon for it.
Thanks for raising this! I've added some comments on here and on the issue... like you say, Just a side note that I've had to make some structural changes to the repo to keep versions more consistent between tests... so this will need to be rebased (sorry to cause hassle!) Another change that has been made on the main today since you raised this is that I've changed how the ArgLineBuilder class works a bit. Originally this was to make it easier to test some behaviours and how they differ based on argline ordering, but it has the side effect that it introduces an internal |
I've been thinking about this and I think that in the long run, it is going to be more beneficial to make a breaking change for this so that the plugin attributes in the config can take an This would be breaking purely on the basis that I'd have to slightly change how URL and Path based plugins get provided in config. I have some code locally that I can push later once I have tidied it up a bit, I will run that past you once it is ready. Going to close this for now as there are some other architectural changes that I will likely include in this solution to keep stuff easy to test going forwards. Thanks for taking the time to put this together though, and thanks for your patience, it is greatly appreciated! |
@ascopes thanks! For that solution, could you also add an option to not fail if the plugin binary is not found? That was another angle that just came up yesterday for me. The current code supports this for missing proto sources, but not missing plugins. (I have optional plugins). |
Will see what I can do, are you able to raise a separate issue for it? |
Done -- #182 |
This is a prototype solution for issue #168.
I would prefer a more cleaned up solution, especially regarding the plugin options.