Refactor ConventionsPlugin class #20805
Conversation
…ConventionsPlugin class
spring-projects-issues
added
the
status: waiting-for-triage
There was a problem hiding this comment.
Thanks for the proposal, @mikesmithson. I've left a few comments for your consideration. In addition to those, if we're going to go down this route, I think the Java-related conventions should be pulled out into a separate class as well. Would you like to update the PR to do that as well?
| * | ||
| * @author Mike Smithson | ||
| */ | ||
| public class MavenPomConventions implements Plugin<Project> { |
There was a problem hiding this comment.
This class doesn't need to be a Plugin. It can also be package-private. The existing AsciidoctorConventions class sets a good precedent here.
There was a problem hiding this comment.
This class applies more than pom conventions. I think MavenPublishingConventions would be a better name.
| @@ -123,53 +116,81 @@ public void apply(Project project) { | |||
|
|
|||
| private void applyJavaConventions(Project project) { | |||
| project.getPlugins().withType(JavaBasePlugin.class, (java) -> { | |||
| project.getPlugins().apply(TestFailuresPlugin.class); | |||
| applyTestFailuresPlugin(project); | |||
There was a problem hiding this comment.
Given that applying the plugin is a single line and wrapping it in a method doesn't make the intent any clearer, I'd prefer to keep it inlined here.
| compile.getOptions().setFork(true); | ||
| compile.getOptions().getForkOptions().setJavaHome(new File(javaHome)); | ||
| compile.getOptions().getForkOptions().setExecutable(javaHome + "/bin/javac"); | ||
| setSourceCompatibility(project); |
There was a problem hiding this comment.
I'd prefer to set the property directly here rather than wrapping it in a method call.
| @@ -111,6 +103,7 @@ | |||
| * | |||
| * @author Andy Wilkinson | |||
| * @author Christoph Dreis | |||
| * @author Mike Smithson | |||
There was a problem hiding this comment.
The javadoc above should be updated to replace the list of actions when the MavenPublishPlugin is applied and to link to the new Maven conventions class instead.
wilkinsona
added
the
status: waiting-for-feedback
|
@wilkinsona - sure, I would be happy to accommodate your requests. I was thinking of going further but wanted to put out this proposal to you to see if there was any interest in pursuing it. |
spring-projects-issues
added
status: feedback-provided
…ated generation to DocumentConventions
|
@wilkinsona - made another push to address your requested changes. |
mikesmithson
changed the title
There was a problem hiding this comment.
Thanks for the updates, @mikesmithson. It's looking pretty good now. I'm not sure I like the separate DocumentConventions class. As mentioned in the comment, I'd prefer to see that logic remain alongside the other Java-related conventions.
| * @author Christoph Dreis | ||
| * @author Mike Smithson | ||
| */ | ||
| class DocumentConventions { |
There was a problem hiding this comment.
I'd prefer to keep these in the same class as the other Java-related conventions.
There was a problem hiding this comment.
OK, I inlined all of the methods in DocumentConventions into JavaConventions.
…line all methods into JavaConventions class
|
latest requested changes are in commit 13e7406. |
wilkinsona
added
type: task
|
Thanks very much, @mikesmithson. The proposed changes are now in master along with a small polishing commit to make things a little more consistent across the three |
|
@wilkinsona - thanks! |
This class was extracted from several private methods in the ConventionsPlugin class. While trying to make a PR for an issue, I had some difficulty following the flow of this class and noticed that it had a lot of private methods that probably should be extracted to a separate class. I found several similar methods related to Maven pom conventions. There are probably a few more, but this one stood out as one obvious candidate.