[GitHub] [maven] gnodet commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
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());
}
```
--
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: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
gnodet commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1321137864
##
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##
@@ -152,16 +149,15 @@ private ClassRealm createRealm(
List parentImports,
Map foreignImports,
List artifacts) {
-Set artifactIds = new LinkedHashSet<>();
-
-List constituents = new ArrayList<>();
+List constituents = new ArrayList<>(artifacts
== null ? 0 : artifacts.size());
-if (artifacts != null) {
+if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
-artifactIds.add(getId(artifact));
if (artifact.getFile() != null) {
constituents.add(new
ArtifactClassRealmConstituent(artifact));
+} else if (logger.isDebugEnabled()) {
+logger.debug(" Excluded: {}", getId(artifact));
Review Comment:
Re-reading this PR before merging, I think this is slightly wrong.
I think artifacts should be considered excluded and logged if
`isProvidedArtifact` returns `true`, which I think is the main reason for the
exclusion, as they kinda are supposed to always point to a file.
--
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: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
gnodet commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1314461642
##
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##
@@ -152,14 +150,15 @@ private ClassRealm createRealm(
List parentImports,
Map foreignImports,
List artifacts) {
-Set artifactIds = new LinkedHashSet<>();
+Set artifactIds = new LinkedHashSet<>(artifacts == null ? 0 :
artifacts.size());
+List constituents = new ArrayList<>(artifacts
== null ? 0 : artifacts.size());
-List constituents = new ArrayList<>();
-
-if (artifacts != null) {
+if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
-artifactIds.add(getId(artifact));
+if (logger.isDebugEnabled()) {
+artifactIds.add(getId(artifact));
+}
Review Comment:
I would not make the computation of the artifactIds condition to the log
level.
##
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##
@@ -301,20 +300,18 @@ private void callDelegates(
}
private Set populateRealm(ClassRealm classRealm,
List constituents) {
-Set includedIds = new LinkedHashSet<>();
+Set includedIds = new LinkedHashSet<>(constituents.size());
-if (logger.isDebugEnabled()) {
-logger.debug("Populating class realm " + classRealm.getId());
-}
+logger.debug("Populating class realm {}", classRealm.getId());
for (ClassRealmConstituent constituent : constituents) {
File file = constituent.getFile();
-String id = getId(constituent);
-includedIds.add(id);
-
if (logger.isDebugEnabled()) {
-logger.debug(" Included: " + id);
+String id = getId(constituent);
Review Comment:
Same here. Move back the `includedIds` computation and simply remove the
`isDebugEnabled()` call.
--
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: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
