gnodet-bot commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4069049959
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/CommonsCliMavenOptions.java:
##########
@@ -215,6 +215,14 @@ public Optional<String> atFile() {
return Optional.empty();
}
+ @Override
+ public Optional<List<String>> skippedPhases() {
+ if (commandLine.hasOption(CLIManager.SKIP_PHASES)) {
+ return
Optional.of(Arrays.asList(commandLine.getOptionValues(CLIManager.SKIP_PHASES)));
Review Comment:
**Whitespace sensitivity bug — silent skip failure.**
`commandLine.getOptionValues()` returns the raw tokens as split by Commons
CLI. With `valueSeparator(',')` + `hasArgs()`, the CLI splits on commas — but
does NOT strip whitespace. A user who writes:
```
mvn verify --skip-phases test, integration-test
```
gets `["test", " integration-test"]` (note the leading space). The
`skippedPhases.contains("integration-test")` check in both
`DefaultLifecycleExecutionPlanCalculator` and `BuildPlanExecutor` then silently
fails to skip `integration-test` — no error, no warning, mojos run anyway.
Fix: strip whitespace at the source:
```suggestion
return
Optional.of(Arrays.stream(commandLine.getOptionValues(CLIManager.SKIP_PHASES))
.map(String::strip)
.collect(java.util.stream.Collectors.toList()));
```
Alternatively, add a warning in the option description (as
`activatedProfiles` does: _"Don't use spaces between commas"_), but stripping
is strictly safer since it handles the common user mistake silently and
correctly.
--
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]
