[GitHub] [maven] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320862296
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -184,14 +189,19 @@ protected AbstractMavenTransferListener(PrintStream out) {
@Override
public void transferInitiated(TransferEvent event) {
+String darkOn = MessageUtils.isColorEnabled() ? ANSI_DARK_SET : "";
Review Comment:
As we know, by default Maven "leaks" your artifact paths, and **only
reason** "Downloading" log line is useful is only this one: it makes user
**aware** Maven is making HTTP request against the logged baseUrl. OTOH, due
overwhelming amount of Maven logging, this information is usually lost, due
log-noise.
To properly fix "leaking", use
[RRF](https://maven.apache.org/resolver/remote-repository-filtering.html) but
that is another thing.
--
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] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320858589
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -184,14 +189,19 @@ protected AbstractMavenTransferListener(PrintStream out) {
@Override
public void transferInitiated(TransferEvent event) {
+String darkOn = MessageUtils.isColorEnabled() ? ANSI_DARK_SET : "";
Review Comment:
> This should be a separate discussion. Foe this we have other listeners.
What other listeners we have for this? WDYM?
--
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] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320858231
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -184,14 +189,19 @@ protected AbstractMavenTransferListener(PrintStream out) {
@Override
public void transferInitiated(TransferEvent event) {
+String darkOn = MessageUtils.isColorEnabled() ? ANSI_DARK_SET : "";
Review Comment:
Well JIRA currently states following:
>transfer message are currently hard to read for many users
> (logging example cut for brevity)
> * it's interweaved into normal build messages
> * users don't really see the difference between "Downloading" (transfert
started, may eventually fail with 404) and "Downloaded" (done successfully)
> * repository id is not so visible in the middle of the message
> * the download url has much info in it to see: base url, groupId as
directory, artifactId, version, and filename
So, based on this above, I assumed, that: is interweave and too chatty, so
cut it down, and if "users don't see the difference", they are right: As I
said, this logger when 5 repositories are defined will emit 5 "Downloading" and
1 "Downloaded", and the one "Downloaded" matters, the 5 "Downloading" does not
(better check with effective-pom what reposes you have instead to repeat it to
each artifact you need to download).
Unsure how JIRA comes to conclusion based on these 4 bullets above "ok,
let's color it!", when it is ONLY solving the last bullet (is not solving it
actually, as when this listener is involved, layout is already applied, and we
do not speak about "artifacts" but about "resources" that are resolved against
some baseUrl/repository, so this is another problem).
--
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] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320858231
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -184,14 +189,19 @@ protected AbstractMavenTransferListener(PrintStream out) {
@Override
public void transferInitiated(TransferEvent event) {
+String darkOn = MessageUtils.isColorEnabled() ? ANSI_DARK_SET : "";
Review Comment:
Well JIRA currently states following:
>transfer message are currently hard to read for many users
> (logging example cut for brevity)
> * it's interweaved into normal build messages
> * users don't really see the difference between "Downloading" (transfert
started, may eventually fail with 404) and "Downloaded" (done successfully)
> * repository id is not so visible in the middle of the message
> * the download url has much info in it to see: base url, groupId as
directory, artifactId, version, and filename
So, based on this above, I assumed, that: is interweave and too chatty, so
cut it down, and if "users don't see the difference", they are right: As I
said, this logger when 5 repositories are defined will emit 5 "Downloading" and
1 "Downloaded", and the one "Downloaded" matters, the 5 "Downloading" does not
(better check with effective-pom what reposes you have instead to repeat it to
each artifact you need to download).
Unsure how JIRA comes to conclusion based on these 4 bullets above "ok,
let's color it!", when it is ONLY solving the last bullet.
--
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] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320858231
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -184,14 +189,19 @@ protected AbstractMavenTransferListener(PrintStream out) {
@Override
public void transferInitiated(TransferEvent event) {
+String darkOn = MessageUtils.isColorEnabled() ? ANSI_DARK_SET : "";
Review Comment:
Well JIRA currently states following:
>transfer message are currently hard to read for many users
> (logging example cut for brevity)
> * it's interweaved into normal build messages
> * users don't really see the difference between "Downloading" (transfert
started, may eventually fail with 404) and "Downloaded" (done successfully)
> * repository id is not so visible in the middle of the message
> * the download url has much info in it to see: base url, groupId as
directory, artifactId, version, and filename
So, based on this above, I assumed, that: is interweave and too chatty, so
cut it down, and if "users don't see the difference", they are right: As I
said, this logger when 5 repositories are defined will emit 5 "Downloading" and
1 "Downloaded", and the one "Downloaded" matters, the 5 "Downloading" does not
(better check with effective-pom what reposes you have).
Unsure how JIRA comes to conclusion based on these 4 bullets above "ok,
let's color it!", when it is ONLY solving the last bullet.
--
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] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320857116
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -214,18 +227,38 @@ public void transferSucceeded(TransferEvent event) {
FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH);
StringBuilder message = new StringBuilder();
-message.append(action).append(' ').append(direction).append('
').append(resource.getRepositoryId());
-message.append(": ");
-
message.append(resource.getRepositoryUrl()).append(resource.getResourceName());
-message.append(" (").append(format.format(contentLength));
+message.append(action).append(darkOn).append('
').append(direction).append(' ');
+message.append(darkOff).append(resource.getRepositoryId());
+message.append(darkOn).append(": ");
+message.append(resource.getRepositoryUrl()).append(darkOff);
+appendResource(message, resource.getResourceName());
+message.append(darkOn).append("
(").append(format.format(contentLength));
long duration = System.currentTimeMillis() -
resource.getTransferStartTime();
if (duration > 0L) {
double bytesPerSecond = contentLength / (duration / 1000.0);
message.append(" at ").append(format.format((long)
bytesPerSecond)).append("/s");
}
-message.append(')');
+message.append(')').append(darkOff);
out.println(message.toString());
}
+
+private void appendResource(StringBuilder message, String resource) {
+if (!MessageUtils.isColorEnabled() ||
resource.endsWith("/maven-metadata.xml")) {
+message.append(resource);
+return;
+}
+// use Maven2 layout for non-metadata files:
https://maven.apache.org/repositories/layout.html
Review Comment:
Please do not make wrong assumptions.
--
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] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320853920
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -184,14 +189,19 @@ protected AbstractMavenTransferListener(PrintStream out) {
@Override
public void transferInitiated(TransferEvent event) {
+String darkOn = MessageUtils.isColorEnabled() ? ANSI_DARK_SET : "";
Review Comment:
If we want to cut logging (and I do agree with @elharo we log way too much),
I'd just remove this method (make it no op).
Or just leave it for PUT (leave it as before), and do not log for the rest.
Reason is simple: if you have 5 reposes, you will have 5 "Downloading"
but only one "Downloaded", and the latter matter.s
--
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] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320854377
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -214,18 +227,37 @@ public void transferSucceeded(TransferEvent event) {
FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH);
StringBuilder message = new StringBuilder();
-message.append(action).append(' ').append(direction).append('
').append(resource.getRepositoryId());
-message.append(": ");
-
message.append(resource.getRepositoryUrl()).append(resource.getResourceName());
-message.append(" (").append(format.format(contentLength));
+message.append(action).append(lowOn).append('
').append(direction).append(' ');
+message.append(lowOff).append(resource.getRepositoryId());
+message.append(lowOn).append(": ");
+message.append(resource.getRepositoryUrl()).append(lowOff);
+appendResource(message, resource.getResourceName());
+message.append(lowOn).append("
(").append(format.format(contentLength));
long duration = System.currentTimeMillis() -
resource.getTransferStartTime();
if (duration > 0L) {
double bytesPerSecond = contentLength / (duration / 1000.0);
message.append(" at ").append(format.format((long)
bytesPerSecond)).append("/s");
}
-message.append(')');
+message.append(')').append(lowOff);
out.println(message.toString());
}
+
+private void appendResource(StringBuilder message, String resource) {
+if (!MessageUtils.isColorEnabled()) {
+message.append(resource);
+return;
+}
+int filename = resource.lastIndexOf('/');
+int version = resource.substring(0, filename).lastIndexOf('/');
+int artifactId = resource.substring(0, version).lastIndexOf('/');
Review Comment:
Layout is delegated to resolver. since Maven3.0. Maven cannot and should not
make any assumptions here, all Maven can do is to control what layout is to be
used with resolver. Doing assumptions like these is plain wrong.
--
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] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320853920
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -184,14 +189,19 @@ protected AbstractMavenTransferListener(PrintStream out) {
@Override
public void transferInitiated(TransferEvent event) {
+String darkOn = MessageUtils.isColorEnabled() ? ANSI_DARK_SET : "";
Review Comment:
If we want to cut logging (and I do agree with @elharo we log way too much),
I'd just remove this method (make it no op).
Or just leave it for PUT (leave it as before), and do not log nothing for
the rest.
Reason is simple: if you have 5 reposes, you will have 5 "Downloading"
but only one "Downloaded", and the latter matter.s
--
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] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320853920
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -184,14 +189,19 @@ protected AbstractMavenTransferListener(PrintStream out) {
@Override
public void transferInitiated(TransferEvent event) {
+String darkOn = MessageUtils.isColorEnabled() ? ANSI_DARK_SET : "";
Review Comment:
If we want to cut logging (and I do agree with @elharo we log way too much),
I'd just remove this method (make it no op).
Or just leave it for PUT (leave it as before), and do not log nothing for
the rest.
Reason is simple: if you have 5 reposes, you will have 5 "Downloading"
but only one "Downloaded", and the latter matter.
--
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] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320853550
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -214,18 +227,38 @@ public void transferSucceeded(TransferEvent event) {
FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH);
StringBuilder message = new StringBuilder();
-message.append(action).append(' ').append(direction).append('
').append(resource.getRepositoryId());
-message.append(": ");
-
message.append(resource.getRepositoryUrl()).append(resource.getResourceName());
-message.append(" (").append(format.format(contentLength));
+message.append(action).append(darkOn).append('
').append(direction).append(' ');
+message.append(darkOff).append(resource.getRepositoryId());
+message.append(darkOn).append(": ");
+message.append(resource.getRepositoryUrl()).append(darkOff);
+appendResource(message, resource.getResourceName());
+message.append(darkOn).append("
(").append(format.format(contentLength));
long duration = System.currentTimeMillis() -
resource.getTransferStartTime();
if (duration > 0L) {
double bytesPerSecond = contentLength / (duration / 1000.0);
message.append(" at ").append(format.format((long)
bytesPerSecond)).append("/s");
}
-message.append(')');
+message.append(')').append(darkOff);
out.println(message.toString());
}
+
+private void appendResource(StringBuilder message, String resource) {
+if (!MessageUtils.isColorEnabled() ||
resource.endsWith("/maven-metadata.xml")) {
+message.append(resource);
+return;
+}
+// use Maven2 layout for non-metadata files:
https://maven.apache.org/repositories/layout.html
Review Comment:
By the way, since Maven3 it is called "default" layout, not "maven2 layout"
(as Maven2 did support "maven1" layout as well, but Maven3 does not).
--
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] cstamas commented on a diff in pull request #1231: MNG-7875 colorize transfer messages
cstamas commented on code in PR #1231:
URL: https://github.com/apache/maven/pull/1231#discussion_r1320853289
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -214,18 +227,38 @@ public void transferSucceeded(TransferEvent event) {
FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH);
StringBuilder message = new StringBuilder();
-message.append(action).append(' ').append(direction).append('
').append(resource.getRepositoryId());
-message.append(": ");
-
message.append(resource.getRepositoryUrl()).append(resource.getResourceName());
-message.append(" (").append(format.format(contentLength));
+message.append(action).append(darkOn).append('
').append(direction).append(' ');
+message.append(darkOff).append(resource.getRepositoryId());
+message.append(darkOn).append(": ");
+message.append(resource.getRepositoryUrl()).append(darkOff);
+appendResource(message, resource.getResourceName());
+message.append(darkOn).append("
(").append(format.format(contentLength));
long duration = System.currentTimeMillis() -
resource.getTransferStartTime();
if (duration > 0L) {
double bytesPerSecond = contentLength / (duration / 1000.0);
message.append(" at ").append(format.format((long)
bytesPerSecond)).append("/s");
}
-message.append(')');
+message.append(')').append(darkOff);
out.println(message.toString());
}
+
+private void appendResource(StringBuilder message, String resource) {
+if (!MessageUtils.isColorEnabled() ||
resource.endsWith("/maven-metadata.xml")) {
+message.append(resource);
+return;
+}
+// use Maven2 layout for non-metadata files:
https://maven.apache.org/repositories/layout.html
Review Comment:
Please don't do this: we want a LONG road from Maven2 to NOT have code (and
assumptions) like this sprinkled across the codebase.
The API offers you repositoryBaseUrl and relative resource path, so just
color those two differently (as I agree, repo URL IS important, it may be
affected by user settings) while resource path IS NOT important
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java:
##
@@ -214,18 +227,38 @@ public void transferSucceeded(TransferEvent event) {
FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH);
StringBuilder message = new StringBuilder();
-message.append(action).append(' ').append(direction).append('
').append(resource.getRepositoryId());
-message.append(": ");
-
message.append(resource.getRepositoryUrl()).append(resource.getResourceName());
-message.append(" (").append(format.format(contentLength));
+message.append(action).append(darkOn).append('
').append(direction).append(' ');
+message.append(darkOff).append(resource.getRepositoryId());
+message.append(darkOn).append(": ");
+message.append(resource.getRepositoryUrl()).append(darkOff);
+appendResource(message, resource.getResourceName());
+message.append(darkOn).append("
(").append(format.format(contentLength));
long duration = System.currentTimeMillis() -
resource.getTransferStartTime();
if (duration > 0L) {
double bytesPerSecond = contentLength / (duration / 1000.0);
message.append(" at ").append(format.format((long)
bytesPerSecond)).append("/s");
}
-message.append(')');
+message.append(')').append(darkOff);
out.println(message.toString());
}
+
+private void appendResource(StringBuilder message, String resource) {
+if (!MessageUtils.isColorEnabled() ||
resource.endsWith("/maven-metadata.xml")) {
+message.append(resource);
+return;
+}
+// use Maven2 layout for non-metadata files:
https://maven.apache.org/repositories/layout.html
Review Comment:
Please don't do this: we went a LONG road from Maven2 to NOT have code (and
assumptions) like this sprinkled across the codebase.
The API offers you repositoryBaseUrl and relative resource path, so just
color those two differently (as I agree, repo URL IS important, it may be
affected by user settings) while resource path IS NOT important
--
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
