gnodet commented on code in PR #2057:
URL: https://github.com/apache/maven/pull/2057#discussion_r1928844335
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/ClingSupport.java:
##########
@@ -60,8 +64,26 @@ private ClingSupport(ClassWorld classWorld, boolean
classWorldManaged) {
* The main entry point.
*/
public int run(String[] args) throws IOException {
+ return run(args, null, null, null);
+ }
+
+ /**
+ * The main entry point with IO management.
+ */
+ public int run(String[] args, @Nullable InputStream in, @Nullable
OutputStream out, @Nullable OutputStream err)
+ throws IOException {
try (Invoker invoker = createInvoker()) {
- return invoker.invoke(parseArguments(args));
+ ParserRequest.Builder parserRequestBuilder =
parserRequestBuilder(args);
+ if (in != null) {
Review Comment:
Aren't those checks useless ?
It's definitely no big deal, but
```
ParserRequest parserRequestBuilder = parserRequestBuilder(args)
.in(in).out(out).err(err)
.build();
```
##########
impl/maven-executor/src/main/java/org/apache/maven/cling/executor/embedded/EmbeddedMavenExecutor.java:
##########
@@ -296,11 +306,18 @@ protected Context doCreate(Path mavenHome,
ExecutorRequest executorRequest) {
try {
if (r.stdoutConsumer().isPresent()
|| r.stderrConsumer().isPresent()) {
+
System.setProperty("org.jline.terminal.output", "out");
Review Comment:
We don't have JLine in the classpath, right ? Else
`TerminalBuilder.PROP_OUTPUT` would be more appropriate.
##########
impl/maven-jline/src/main/java/org/apache/maven/jline/MessageUtils.java:
##########
@@ -49,8 +49,11 @@ public static void systemInstall() {
public static void systemInstall(Consumer<TerminalBuilder>
builderConsumer, Consumer<Terminal> terminalConsumer) {
MessageUtils.terminal = new FastTerminal(
() -> {
- TerminalBuilder builder =
- TerminalBuilder.builder().name("Maven").dumb(true);
+ TerminalBuilder builder = TerminalBuilder.builder()
+ .jansi(false)
+ .jna(false)
+ .jni(true)
+ .name("Maven");
Review Comment:
We may still want to have `dumb(true)`.
For example if we are on an unsupported os/architecture combination, we'll
end up with a dumb terminal, but with an annoying warning (and nothing the user
can do)...
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java:
##########
@@ -279,18 +290,16 @@ protected void createTerminal(C context) {
// so boot it asynchronously
context.closeables.add(MessageUtils::systemUninstall);
MessageUtils.registerShutdownHook(); // safety belt
- if (context.coloredOutput != null) {
- MessageUtils.setColorEnabled(context.coloredOutput);
- }
} else {
- if (context.coloredOutput != null) {
- MessageUtils.setColorEnabled(context.coloredOutput);
- }
+ MessageUtils.setColorEnabled(Objects.requireNonNullElseGet(
+ context.coloredOutput, () ->
!Terminal.TYPE_DUMB.equals(context.terminal.getType())));
}
}
protected void doConfigureWithTerminal(C context, Terminal terminal) {
context.terminal = terminal;
+ MessageUtils.setColorEnabled(Objects.requireNonNullElseGet(
Review Comment:
Why do we have two calls to `MessageUtils.setColorEnabled` ? Shouldn't only
one be sufficient ?
--
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]
