gnodet-bot commented on code in PR #13498:
URL: https://github.com/apache/maven/pull/13498#discussion_r4277353901
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/PluginParameterExpressionEvaluator.java:
##########
@@ -240,7 +247,8 @@ public Object evaluate(String expr, Class<?> type) throws
ExpressionEvaluationEx
try {
int pathSeparator = expression.indexOf('/');
Review Comment:
🔎 **Redundant null ternary** — dead code
The outer `else if` condition already guarantees `mojoDescriptor != null`,
so the ternary `mojoDescriptor != null ? mojoDescriptor.getPluginDescriptor() :
null` is unreachable in the `false` branch.
`ReflectionValueExtractor.evaluate()` accepts `@Nullable Object root` so this
is safe either way, but the redundant check is confusing — it implies
`mojoDescriptor` could be null here, which it can't.
```suggestion
PluginDescriptor pluginDescriptor =
mojoDescriptor.getPluginDescriptor();
```
##########
impl/maven-core/src/test/java/org/apache/maven/plugin/PluginParameterExpressionEvaluatorTest.java:
##########
@@ -94,6 +94,24 @@ void testPluginDescriptorExpressionReference() throws
Exception {
"${plugin} expression does not return plugin descriptor.");
}
+ @Test
+ void testEvaluationWithoutMojoExecution() throws Exception {
Review Comment:
⚠️ **Compilation failure: duplicate method**
`testEvaluationWithoutMojoExecution()` is defined twice in this class. The
merge of `origin/master` into this PR branch brought in the richer version
added by the upstream fix commit (`dd815500df`), while the simpler variant
added here at line 98 was not removed.
```
[ERROR] method testEvaluationWithoutMojoExecution() is already defined in
class PluginParameterExpressionEvaluatorTest
```
The two variants cover different scenarios: this one tests `${session}`,
`${project}`, literals, and `${mojo.*}`/`${plugin.*}` expressions all in one.
The upstream version splits that into two focused tests
(`testEvaluationWithoutMojoExecution` and
`testMojoAndPluginExpressionsWithoutMojoExecution`) using a proper Plexus
container session rather than `newMavenSession()`.
The simplest fix is to remove this method and keep the two upstream tests.
Alternatively, rename this method and keep the simpler `newMavenSession()`
variant alongside the fuller ones.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
