gnodet-bot commented on code in PR #13667:
URL: https://github.com/apache/maven/pull/13667#discussion_r4343021928


##########
impl/maven-core/src/main/java/org/apache/maven/logging/ProjectBuildLogAppender.java:
##########
@@ -141,9 +141,11 @@ public static void setReportCapture(Consumer<LogEvent> 
capture) {
     }
 
     private final BuildEventListener buildEventListener;
+    private final boolean suppressInlineWarnings;
 
-    public ProjectBuildLogAppender(BuildEventListener buildEventListener) {
+    public ProjectBuildLogAppender(BuildEventListener buildEventListener, 
String warningMode) {

Review Comment:
   💡 **Nit:** The new constructor parameter lacks Javadoc. Since this 
constructor is `public` and controls filtering behaviour, the `@param 
warningMode` should document the accepted values and their effect.
   
   ```suggestion
       /**
        * Creates a new appender that forwards log events to the given listener.
        *
        * @param buildEventListener the listener to forward events to
        * @param warningMode the warning display mode ({@code "summary"}, 
{@code "all"},
        *                    {@code "none"}, {@code "fail"}); {@code "summary"} 
suppresses
        *                    inline WARN events while preserving them for the 
build report
        */
       public ProjectBuildLogAppender(BuildEventListener buildEventListener, 
String warningMode) {
   ```



##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java:
##########
@@ -353,8 +353,9 @@ protected final void createTerminal(C context) {
             MessageUtils.registerShutdownHook(); // safety belt
 
             // Create the build log appender; also sets MavenSimpleLogger sink

Review Comment:
   💡 **Nit:** The `"summary"` fallback is hardcoded here, but 
`Options.warningMode()` Javadoc already documents `"summary"` as the default. 
`BuildReportCollector` independently hardcodes the same default, creating three 
divergent owners of this knowledge. Consider extracting a constant (e.g. 
`WarningMode.DEFAULT = "summary"`) so a future change to the default doesn't 
require hunting down all call sites.
   
   Not a blocker for this PR, but worth tracking.



-- 
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]

Reply via email to