gnodet commented on PR #13392:
URL: https://github.com/apache/maven/pull/13392#issuecomment-6654370488
Good feature addition overall — the lifecycle filter is a useful escape
hatch. A few issues to address before merge.
---
**[MUST] `@since` tag wrong in all new Javadoc — should be `4.2.0`**
Five places use `@since 4.1.0` but this PR targets milestone **4.2.0**
(`master`):
- `Constants.java` — `MAVEN_LIFECYCLE_FILTER`
- `CoordinatePredicate.java` — class Javadoc
- `FilterPredicate.java` — interface Javadoc
- `MojoExecutionFilter.java` — class Javadoc
- `PhasePredicate.java` — class Javadoc
Please change all five to `@since 4.2.0`.
---
**[MUST] `ExecutionEventLogger.mojoSkipped` regression — offline-skip
warning silenced**
Both `compat/maven-embedder` and `impl/maven-cli`
`ExecutionEventLogger.mojoSkipped` previously warned unconditionally when a
goal requires online mode:
```java
// before
if (logger.isWarnEnabled()) {
logger.warn("Goal '{}' requires online mode for execution but Maven is
currently offline, skipping", ...);
```
This PR changes both to:
```java
// after
if (logger.isWarnEnabled() && event.getSession().isOffline()) {
```
The added `&& event.getSession().isOffline()` guard makes no sense here. The
warn message *only fires* when the goal is being skipped because the session is
offline — the offline condition is what caused the skip in the first place.
Adding the guard won't change normal behaviour, but it leaks an incorrect
mental model and would silently suppress the warning if `isOffline()` ever
returns a stale value. The right approach is to handle the two skip causes in
`mojoSkipped` separately (or just leave the offline path alone and add a new
filter-skip notification). Please revert these two hunks.
---
**[SHOULD] Skip log message is unhelpful — logs the property name, not the
matching predicate**
```java
LOGGER.info("Skipping {} (filtered by {})", mojoExecution,
MojoExecutionFilter.PROPERTY_NAME);
```
`MojoExecutionFilter.PROPERTY_NAME` resolves to the string
`"maven.lifecycle.filter"`. When several predicates are configured, the user
has no idea which one matched. Both `CoordinatePredicate` and `PhasePredicate`
have well-implemented `toString()` methods that would surface the specific
pattern. Consider returning the matching predicate from `matches()` (or
changing the return type to `Optional<FilterPredicate>`) so the log can say
which predicate triggered the skip.
---
**[NITS]**
- `INFO` level for filter-skipped mojos may be too quiet when running with
`-q` (which suppresses INFO). Users who enable the filter and run a quiet build
won't see that everything was skipped. `WARN` or at least a summary at the end
of the build would be more user-friendly.
- `java.util.Properties` import in `MojoExecutor` is unused if
`session.getUserProperties()` can be refactored to avoid the intermediate
variable (minor).
---
> 🤖 This review was posted by [Hermes Agent](https://github.com/gnodet) on
behalf of @gnodet.
--
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]
