Conversation
OverviewThe Single-Module ProjectsFor single-module Maven projects, the annotation processor is added to the
Multi-Module ProjectsFor multi-module Maven projects, the annotation processor is added to the
Version HandlingVersion handling applies to both single-module and multi-module projects:
|
…nnotationProcessor`
…vior Restructure tests into @nested classes to clearly define expected behavior: - SingleModuleProject: add to build/plugins - MultiModuleProject: add to build/pluginManagement/plugins in root pom only Remove redundant tests, keeping @DocumentExample and version handling tests.
…d annotation processors where expected
ade42af to
e318692
Compare
There was a problem hiding this comment.
Could we generalize this visitor and the AddPlugin and AddManagedPlugin recipes into a single recipe that takes an xpath expression as input to determine whether to add a plugin to build/plugin or pluginManagement? We could define that option with allowed values to prevent someone from using the recipe with an odd xpath expression
There was a problem hiding this comment.
We could define that option with allowed values to prevent someone from using the recipe with an odd xpath expression
Sorry, I don't understand this sentence.
I can help you understand my decision for two recipes using the same visitor:
- I don't want to break existing usages of
AddPluginwith a new constructor, as its central - Both recipes have fixed XPaths where plugins are added as they are defined by Maven's POM model;
build/pluginsvs.build/pluginManagement/pluginsthere is no need for a custom XPath. - The visitor takes a
booleanflag instead of an XPath because we need the knowledge if it is a managed or build plugin not only for the location where to add thepluginbut also for the case that thebuildtags do not contain thepluginsorpluginMangement/pluginspath. - Reusing the
AddPluginorAddManagedPluginin imperative recipes is easier with a dedicated visitor, and it also reduces the code/test to maintain.
|
What if the root pom is only an aggregating pom, but not a parent pom? Iirc we can determine whether a pom is an aggregating pom and/or a parent pom. We should probably aim to add to the parent pom in this case 🙈 |
|
Good call, this is a valid setup that, especially in legacy code, is used. This reconsideration should address these concerns. ScenariosScenario 1: Single module Scenario 2: Separated aggregator/parent Scenario 3: Combined aggregator/parent Decision TreePhases
Target Matrix
Other decision
|
AddAnnotationProcessor|
@timtebeek @Laurens-W pulling back to draft due to the substantial logic refinements for Done, RFR again |
# Conflicts: # rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv
|
There's some helper methods in |
|
|
||
| @Value | ||
| @EqualsAndHashCode(callSuper = false) | ||
| public class AddPluginVisitor extends MavenIsoVisitor<ExecutionContext> { |
There was a problem hiding this comment.
Bit late now, but can we limit visibility by default on new classes? I don't see an immediate need for this to be part of the exposed API. I'd rather we open up when those needs trickle in.
What's changed?
From the tests,
AddAnnotationProcessordeals with managed plugins and has the assumption, that the tag/project/build/plugins/plugin/configuration/annotationProcessorPathsis present.In many cases in the Java 23 migration, all the annotation processors have to be added, so this assumption is weak.
The
MavenPlugintrait got an improvement to be able to match plugins and managed plugins more precisely, even with group and artifact id.The
AddPluginrecipes visitor got extracted and is reused to have aAddManagedPluginrecipe.Both backward compatible, proven with tests.
What's your motivation?
MavenPlugintrait for more precise visitors.Anything in particular you'd like reviewers to focus on?
Anyone you would like to review specifically?
@Laurens-W @timtebeek
Have you considered any alternatives or workarounds?
Imperative recipe every time we have want to configure an annotation processor safely.
Any additional context
We expect a behavior change here; see the comment for the logic.
Checklist