michael-o commented on a change in pull request #675:
URL: https://github.com/apache/maven/pull/675#discussion_r805389880
##########
File path:
maven-model-builder/src/main/java/org/apache/maven/model/interpolation/AbstractStringBasedModelInterpolator.java
##########
@@ -85,13 +79,22 @@
private final PathTranslator pathTranslator;
private final UrlNormalizer urlNormalizer;
+ @Inject
+ private ModelVersionProcessor versionProcessor;
+
@Inject
public AbstractStringBasedModelInterpolator( PathTranslator
pathTranslator, UrlNormalizer urlNormalizer )
{
this.pathTranslator = pathTranslator;
this.urlNormalizer = urlNormalizer;
}
+ public AbstractStringBasedModelInterpolator setVersionPropertiesProcessor(
ModelVersionProcessor processor )
Review comment:
I do not fully understand why this is a setter, but the private field is
injected. Why not inject on directly on the ctor? At the end this is a required
field not optional, no?
##########
File path:
maven-model-builder/src/main/java/org/apache/maven/model/interpolation/AbstractStringBasedModelInterpolator.java
##########
@@ -85,13 +79,22 @@
private final PathTranslator pathTranslator;
private final UrlNormalizer urlNormalizer;
+ @Inject
+ private ModelVersionProcessor versionProcessor;
+
@Inject
public AbstractStringBasedModelInterpolator( PathTranslator
pathTranslator, UrlNormalizer urlNormalizer )
{
this.pathTranslator = pathTranslator;
this.urlNormalizer = urlNormalizer;
}
+ public AbstractStringBasedModelInterpolator setVersionPropertiesProcessor(
ModelVersionProcessor processor )
Review comment:
Please switch to ctor injection, this will give you two benefits:
1. All required objects are wired and are not optional
2. You can wire manually for testing if necessary, opposite of field
injection
##########
File path:
maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
##########
@@ -89,6 +85,15 @@
private final Set<String> validProfileIds = new HashSet<>();
+ @Inject
+ private ModelVersionProcessor versionProcessor;
Review comment:
Any reason why this is not on ctor injection?
##########
File path:
maven-model-builder/src/main/java/org/apache/maven/model/interpolation/AbstractStringBasedModelInterpolator.java
##########
@@ -86,10 +80,15 @@
private final UrlNormalizer urlNormalizer;
@Inject
Review comment:
This `@Inject` is obsolete now. Move the new field up to the other
private finals.
##########
File path:
maven-model-builder/src/main/java/org/apache/maven/model/interpolation/StringSearchModelInterpolator.java
##########
@@ -68,14 +68,15 @@
}
@Inject
- public StringSearchModelInterpolator( PathTranslator pathTranslator,
UrlNormalizer urlNormalizer )
+ public StringSearchModelInterpolator( PathTranslator pathTranslator,
UrlNormalizer urlNormalizer,
+ ModelVersionProcessor processor )
{
- super( pathTranslator, urlNormalizer );
+ super( pathTranslator, urlNormalizer, processor );
}
StringSearchModelInterpolator()
{
- super( null, null );
+ super( null, null, null );
Review comment:
@cstamas Can you come up with a reason why we have this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
