[
https://issues.apache.org/jira/browse/MNG-7866?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17764981#comment-17764981
]
ASF GitHub Bot commented on MNG-7866:
-------------------------------------
gnodet commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1325372519
##########
maven-core/src/main/java/org/apache/maven/internal/aether/LoggingRepositoryListener.java:
##########
@@ -63,48 +63,47 @@ public void metadataResolved(RepositoryEvent event) {
public void metadataInvalid(RepositoryEvent event) {
Exception exception = event.getException();
- StringBuilder buffer = new StringBuilder(256);
- buffer.append("The metadata ");
+ Object metadata;
if (event.getMetadata().getFile() != null) {
- buffer.append(event.getMetadata().getFile());
+ metadata = event.getMetadata().getFile();
} else {
- buffer.append(event.getMetadata());
+ metadata = event.getMetadata();
}
+ String errorType = " is invalid";
if (exception instanceof FileNotFoundException) {
- buffer.append(" is inaccessible");
- } else {
- buffer.append(" is invalid");
+ errorType = " is inaccessible";
}
+ String msg = "";
if (exception != null) {
- buffer.append(": ");
- buffer.append(exception.getMessage());
+ msg = ": " + exception.getMessage();
}
if (logger.isDebugEnabled()) {
- logger.warn(buffer.toString(), exception);
+ logger.warn("The metadata {} {}{}", metadata, errorType, msg,
exception);
} else {
- logger.warn(buffer.toString());
+ logger.warn("The metadata {} {}{}", metadata, errorType, msg);
}
}
@Override
public void artifactDescriptorInvalid(RepositoryEvent event) {
- StringBuilder buffer = new StringBuilder(256);
- buffer.append("The POM for ");
- buffer.append(event.getArtifact());
- buffer.append(" is invalid, transitive dependencies (if any) will not
be available");
-
if (logger.isDebugEnabled()) {
- logger.warn(buffer + ": " + event.getException().getMessage());
+ logger.warn(
+ "The POM for {} is invalid, transitive dependencies (if
any) will not be available: {}",
+ event.getArtifact(),
+ event.getException().getMessage());
} else {
- logger.warn(buffer + ", enable verbose output (-X) for more
details");
+ logger.warn(
+ "The POM for {} is invalid, transitive dependencies (if
any) will not be available, enable "
+ + "verbose output (-X) for more details",
+ event.getArtifact());
Review Comment:
The code above is wrong and does not make much sense. To be consistent with
other parts of this class, the whole method should look like:
```
if (logger.isDebugEnabled()) {
logger.warn(
"The POM for {} is invalid, transitive dependencies (if any)
will not be available: {}",
event.getArtifact(),
event.getException().getMessage(),
event.getException());
} else {
logger.warn(
"The POM for {} is invalid, transitive dependencies (if any)
will not be available: {}",
event.getArtifact(),
event.getException().getMessage());
}
```
> Improvements to the logging API usage (technical debt)
> ------------------------------------------------------
>
> Key: MNG-7866
> URL: https://issues.apache.org/jira/browse/MNG-7866
> Project: Maven
> Issue Type: Improvement
> Components: Core
> Affects Versions: 4.0.0-alpha-2
> Reporter: sebastien
> Priority: Minor
> Fix For: 4.0.0-alpha-8
>
>
> Since maven 4 is now using the Slf4J logger API, some logging code can be
> improved.
> Typical improvements are:
> * Use message formats with placeholders to avoid premature formatting and
> avoid the unnecessary garbage when then log level is disabled. Example :
>
> {code:java}
> logger.debug("Toolchains configuration was not found at " +
> userToolchainsFile);
> {code}
> can be replaced with :
> {code:java}
> logger.debug("Toolchains configuration was not found at {}",
> userToolchainsFile);{code}
> * Guarding some logging statements with conditionals on isXXXXEnabled() to
> avoid unnecessary garbage when then log level is disabled. Useful when some
> formatting must be done outside the logger call. Example :
>
> {code:java}
> } else {
> Lifecycle original = phaseToLifecycleMap.get(phase);
> logger.warn("Duplicated lifecycle phase " + phase + ".
> Defined in " + original.getId()
> + " but also in " + lifecycle.getId());
> }
> {code}
> can be replaced with the following code to avoid the cost of the map lookup :
>
> {code:java}
> } else if (logger.isWarnEnabled()) {
> Lifecycle original = phaseToLifecycleMap.get(phase);
> logger.warn(
> "Duplicated lifecycle phase {}. Defined in {} but
> also in {}",
> phase,
> original.getId(),
> lifecycle.getId());
> }
> {code}
> * Remove some unneeded conditional guarding to avoid testing twice if the
> log level is enabled, like for example :
>
> {code:java}
> if (logger.isDebugEnabled()) {
> logger.debug("Lifecycle " + lifecycle);
> }
> {code}
> can be replaced with :
> {code:java}
> logger.debug("Lifecycle {}", lifecycle);{code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
