gnodet-bot commented on code in PR #13569:
URL: https://github.com/apache/maven/pull/13569#discussion_r4294183598
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeStrategy.java:
##########
@@ -84,6 +84,17 @@
*/
public abstract class AbstractUpgradeStrategy implements UpgradeStrategy {
+ /**
+ * Logs a change using dry-run wording when the goal is checking rather
than saving.
+ */
+ protected void logChange(UpgradeContext context, String description) {
+ if (context.isDryRun()) {
+ context.action(description + " would be applied");
+ } else {
+ context.success(description + " applied");
+ }
+ }
+
Review Comment:
⚠️ **Garbled output**: this helper appends `" applied"` (or `" would be
applied"`) to any description string, but ~10 of 14 call sites pass past-tense
or passive-voice sentences, producing output like `"Maven 4 compatibility
issues fixed applied"` and `"Maven 4 compatibility issues fixed would be
applied"`.
The helper works correctly only for noun phrases (e.g. `"Model upgrade"`,
`"Plugin upgrades"`). All call sites using past-participle sentences need their
descriptions changed to noun form to produce grammatical output:
```suggestion
protected void logChange(UpgradeContext context, String description) {
if (context.isDryRun()) {
context.action(description + " would be applied");
} else {
context.success(description + " applied");
}
}
```
(No change to the method body needed — the fix is at each call site's
description string.)
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/StrategyOrchestrator.java:
##########
@@ -151,14 +151,19 @@ private void logStrategyResult(UpgradeContext context,
UpgradeStrategy strategy,
context.failure("Strategy completed with errors");
context.indent();
context.info("Processed: " + result.processedPoms().size() + "
POMs");
- context.info("Modified: " + result.modifiedPoms().size() + "
POMs");
+ context.info((context.isDryRun() ? "Would modify: " : "Modified: ")
+ + result.modifiedPoms().size() + " POMs");
context.failure("Errors: " + result.errorPoms().size() + " POMs");
context.unindent();
} else if (!result.modifiedPoms().isEmpty()) {
context.success("Strategy completed successfully");
context.indent();
context.info("Processed: " + result.processedPoms().size() + "
POMs");
- context.success("Modified: " + result.modifiedPoms().size() + "
POMs");
+ if (context.isDryRun()) {
+ context.action(result.modifiedPoms().size() + " POM(s) would
be modified");
+ } else {
Review Comment:
💡 **Inconsistent pluralization**: dry-run branch uses `"POM(s)"` while the
else branch on line 166 uses `"POMs"`. Consider aligning to one form:
```suggestion
context.action(result.modifiedPoms().size() + " POMs would
be modified");
```
--
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]
