Copilot commented on code in PR #709:
URL:
https://github.com/apache/maven-checkstyle-plugin/pull/709#discussion_r4248064544
##########
src/main/java/org/apache/maven/plugins/checkstyle/exec/DefaultCheckstyleExecutor.java:
##########
@@ -219,16 +221,15 @@ public CheckstyleResults
executeCheckstyle(CheckstyleExecutorRequest request)
message.append(" ruleset.");
if (request.isFailsOnError()) {
- // TODO: should be a failure, not an error. Report is not
meant to
- // throw an exception here (so site would
- // work regardless of config), but should record this
information
- throw new CheckstyleExecutorException(message.toString());
+ // Record rather than throw so the report goal can still
generate
+ // the site. The check goal turns this into a
MojoFailureException.
+ results.setFailsOnErrorMessage(message.toString());
} else {
logger.info(message.toString());
}
}
- return checkerListener.getResults();
+ return results;
Review Comment:
With `failsOnError=true`, the executor no longer throws and also no longer
logs the violation summary. For the report goal this can make runs appear
successful/silent in logs even when violations exist (previously an
exception/stack trace would surface the message). Consider logging the message
(e.g., at WARN/INFO) in addition to recording it, so users still get
observability while preserving report generation.
##########
src/main/java/org/apache/maven/plugins/checkstyle/exec/CheckstyleResults.java:
##########
@@ -118,4 +120,12 @@ public Configuration getConfiguration() {
public void setConfiguration(Configuration configuration) {
this.configuration = configuration;
}
+
+ public String getFailsOnErrorMessage() {
+ return failsOnErrorMessage;
+ }
+
+ public void setFailsOnErrorMessage(String failsOnErrorMessage) {
+ this.failsOnErrorMessage = failsOnErrorMessage;
+ }
Review Comment:
The newly added `failsOnErrorMessage` becomes part of a public results
object but currently has no Javadoc describing its contract (when it is set,
whether it may be empty, and which goals are expected to interpret it). Adding
brief Javadoc on the field/accessors would make the intended semantics explicit
and reduce the chance of future misuse.
##########
src/main/java/org/apache/maven/plugins/checkstyle/exec/CheckstyleResults.java:
##########
@@ -38,6 +38,8 @@ public class CheckstyleResults {
private Configuration configuration;
+ private String failsOnErrorMessage;
Review Comment:
The newly added `failsOnErrorMessage` becomes part of a public results
object but currently has no Javadoc describing its contract (when it is set,
whether it may be empty, and which goals are expected to interpret it). Adding
brief Javadoc on the field/accessors would make the intended semantics explicit
and reduce the chance of future misuse.
--
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]
