gnodet-bot commented on code in PR #13136:
URL: https://github.com/apache/maven/pull/13136#discussion_r4015728286
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/event/ExecutionEventLogger.java:
##########
@@ -266,6 +291,12 @@ private void logReactorSummary(MavenSession session) {
}
}
+ private record ReactorSummaryRequest(
+ List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean
isSingleVersion, boolean hasExceptions) {}
Review Comment:
đ§ Same mutable-`StringBuilder`-in-record issue as `compat` copy â apply the
same fix (allocate locally in `logReactorSummaryGroup`, drop the field from
`ReactorSummaryRequest`).
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java:
##########
@@ -199,31 +200,51 @@ private void logReactorSummary(MavenSession session) {
List<MavenProject> projects = session.getProjects();
- StringBuilder buffer = new StringBuilder(128);
-
String skippedMessage = builder().warning("SKIPPED").build();
String successMessage = builder().success("SUCCESS").build();
String failureMessage = builder().failure("FAILURE").build();
String unknownMessage = builder().warning("UNKNOWN").build();
- boolean lastWasSkipped = false;
+ List<ReactorSummaryEntry> entries = new ArrayList<>(projects.size());
for (MavenProject project : projects) {
BuildSummary buildSummary = result.getBuildSummary(project);
String statusMessage;
- boolean shouldSkip = result.hasExceptions();
- if (buildSummary == null) {
- statusMessage = skippedMessage;
- } else if (buildSummary instanceof BuildSuccess) {
+ int group;
+ if (buildSummary instanceof BuildSuccess) {
statusMessage = successMessage;
+ group = 1;
} else if (buildSummary instanceof BuildFailure) {
statusMessage = failureMessage;
- shouldSkip = false;
+ group = 2;
+ } else if (buildSummary == null) {
+ statusMessage = skippedMessage;
+ group = 0;
} else {
statusMessage = unknownMessage;
+ group = 0;
Review Comment:
âšī¸ **`UNKNOWN` buildSummary silently suppressed on failure â still present**
The author marked this as "acceptable". The concern is forward-looking: if a
third `BuildSummary` subclass is ever added (e.g. `BuildCancelled`), any such
result will be swallowed into the `...` placeholder on failure builds, with no
visible trace in the reactor summary. Assigning it to `group = 2` (shown last,
alongside failures) would make this branch safe by default at zero cost:
```suggestion
statusMessage = unknownMessage;
group = 2;
```
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java:
##########
@@ -261,6 +286,12 @@ private void logReactorSummary(MavenSession session) {
}
}
+ private record ReactorSummaryRequest(
+ List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean
isSingleVersion, boolean hasExceptions) {}
Review Comment:
đ§ **Mutable `StringBuilder` inside `record` â still present**
The author responded that this is a checkstyle workaround. Understood â but
the checkstyle constraint (if it is a method length rule or similar) applies
only to the `logReactorSummary` method, not to `logReactorSummaryGroup`. The
`StringBuilder` can be allocated locally at the top of `logReactorSummaryGroup`
(where it is already immediately read from the record) without touching
checkstyle-sensitive code:
```suggestion
private record ReactorSummaryRequest(
List<ReactorSummaryEntry> entries, boolean isSingleVersion,
boolean hasExceptions) {}
```
Then in `logReactorSummaryGroup`, the first line already reads
`StringBuilder buffer = request.buffer()` â just change that to `StringBuilder
buffer = new StringBuilder(128)` and remove the field from the record. The call
site becomes:
```java
ReactorSummaryRequest request =
new ReactorSummaryRequest(entries, isSingleVersion,
result.hasExceptions());
```
This eliminates mutable state from a value-type record with no checkstyle
impact. If there is a specific checkstyle rule blocking this, please point to
it â I cannot identify one that would fire here.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/event/ExecutionEventLogger.java:
##########
@@ -204,31 +205,51 @@ private void logReactorSummary(MavenSession session) {
List<MavenProject> projects = session.getProjects();
- StringBuilder buffer = new StringBuilder(128);
-
String skippedMessage = builder().warning("SKIPPED").build();
String successMessage = builder().success("SUCCESS").build();
String failureMessage = builder().failure("FAILURE").build();
String unknownMessage = builder().warning("UNKNOWN").build();
- boolean lastWasSkipped = false;
+ List<ReactorSummaryEntry> entries = new ArrayList<>(projects.size());
for (MavenProject project : projects) {
BuildSummary buildSummary = result.getBuildSummary(project);
String statusMessage;
- boolean shouldSkip = result.hasExceptions();
- if (buildSummary == null) {
- statusMessage = skippedMessage;
- } else if (buildSummary instanceof BuildSuccess) {
+ int group;
+ if (buildSummary instanceof BuildSuccess) {
statusMessage = successMessage;
+ group = 1;
} else if (buildSummary instanceof BuildFailure) {
statusMessage = failureMessage;
- shouldSkip = false;
+ group = 2;
+ } else if (buildSummary == null) {
+ statusMessage = skippedMessage;
+ group = 0;
} else {
statusMessage = unknownMessage;
+ group = 0;
Review Comment:
âšī¸ Same `UNKNOWN`â`group = 0` silent-suppression issue as `compat` copy â
consider `group = 2` for safety.
--
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]
