gnodet-bot commented on code in PR #13136:
URL: https://github.com/apache/maven/pull/13136#discussion_r4014184425
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java:
##########
@@ -261,6 +282,12 @@ private void logReactorSummary(MavenSession session) {
}
}
+ private record ReactorSummaryRequest(
+ List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean
isSingleVersion, boolean hasExceptions) {}
Review Comment:
đ§ **Design: mutable `StringBuilder` inside a `record`**
A `record` is a value-based type; it shouldn't hold mutable state.
`ReactorSummaryRequest` carries a `StringBuilder` that is mutated by all three
`logReactorSummaryGroup` calls (via `append`/`setLength`). The pattern is
functional because `setLength(0)` resets it between entries, but it's
surprising: a caller reading the record declaration would reasonably expect it
to be immutable.
Simpler and cleaner: allocate the `StringBuilder` locally at the top of
`logReactorSummaryGroup` and drop the field from the record entirely:
```suggestion
private record ReactorSummaryRequest(
List<ReactorSummaryEntry> entries, boolean isSingleVersion,
boolean hasExceptions) {}
```
Then in `logReactorSummaryGroup`:
```java
private void logReactorSummaryGroup(ReactorSummaryRequest request, int
group) {
StringBuilder buffer = new StringBuilder(128);
...
}
```
And update the call site:
```java
ReactorSummaryRequest request =
new ReactorSummaryRequest(entries, isSingleVersion,
result.hasExceptions());
```
This eliminates the shared mutable state and makes the method independently
reentrant.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/event/ExecutionEventLogger.java:
##########
@@ -266,6 +287,12 @@ private void logReactorSummary(MavenSession session) {
}
}
+ private record ReactorSummaryRequest(
+ List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean
isSingleVersion, boolean hasExceptions) {}
Review Comment:
đ§ Same `StringBuilder`-in-record issue as in the `compat` copy. Apply the
same fix here (local `StringBuilder` in `logReactorSummaryGroup`, drop from
`ReactorSummaryRequest`).
##########
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` issue as in the `compat` copy.
##########
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` results silently suppressed on failure**
The `else` branch assigns `group = 0` with `statusMessage = unknownMessage`.
When `hasExceptions() == true`, any non-null, non-`BuildSuccess`,
non-`BuildFailure` `BuildSummary` will be silently swallowed into the `...`
placeholder (group 0 suppression) instead of appearing in the summary.
`BuildSummary` is currently `abstract` with only `BuildSuccess` and
`BuildFailure` as concrete subclasses, so this branch is dead code in practice.
But it's worth noting: if a third subclass is ever added, `UNKNOWN` results
will vanish on failure builds. Consider assigning them to `group = 2` (shown
last, alongside failures) to make the `else` branch safe by default.
--
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]
