Skip to content

Commit

Permalink
Refactor CLI printing and colors
Browse files Browse the repository at this point in the history
Printing from the CLI previously often resulted in hard to read
output because messages were flushed on each newline rather than
each log message/write to stdout/stderr, causing contiguous strings of
text that contain newlines (like model validation events) to be
interleaved between threads. This change updates the CLI to use a
PrintWriter instead of a PrintStream to fix this. Additionally, added
a new API for creating grouped sections of text from a CliPrinter that
includes styling methods.

Forcing color/no_color is now done only through environment variables
(FORCE_COLOR, NO_COLOR). This matches lots of other CLI tools, and removes
the clutter of the old options from help output. If the old options are
used, a warning is logged and they are ignored.

The way in which colors are detected and rendered is updated too. This
change introduces an Ansi enum that contains AUTO, FORCE_COLOR, and
NO_COLOR members. AUTO will auto-detect whether colors are supported
using a heuristic, and dispatch to FORCE_COLOR and NO_COLOR based on if
colors are supported. CliPrinter instances have a reference to Ansi and
use it to style text + provide a public API to get the Ansi color setting.

The hueristic used to detect colors is now:
1. Is FORCE_COLOR set? colors
2. Is NO_COLOR set? no colors
3. Is TERM set and set to "dumb"? no colors
4. Is TERM null and the OS is Windows? no colors
5. Is a System console detected (i.e., interactive CLI that isn't
   redirected)? colors
6. no colors

I also updated checkstyle to allow for empty methods with braces on
the same line to accomodate some of the implementations I was added.
I then went and made similar changes to classes in the CLI package
to use this approach.
  • Loading branch information
mtdowling committed Dec 10, 2022
1 parent f61df7e commit 8c52252
Show file tree
Hide file tree
Showing 20 changed files with 515 additions and 293 deletions.
8 changes: 6 additions & 2 deletions config/checkstyle/checkstyle.xml
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,12 @@
<module name="RightCurly">
<property name="id" value="RightCurlyAlone"/>
<property name="option" value="alone"/>
<property name="tokens"
value="CLASS_DEF, METHOD_DEF, LITERAL_FOR, LITERAL_WHILE, STATIC_INIT, INSTANCE_INIT"/>
<property name="tokens" value="LITERAL_FOR, LITERAL_WHILE, STATIC_INIT, INSTANCE_INIT"/>
</module>
<module name="RightCurly">
<property name="id" value="RightCurlyAloneOrSingle"/>
<property name="option" value="alone_or_singleline"/>
<property name="tokens" value="CLASS_DEF, METHOD_DEF"/>
</module>

<!-- Checks for common coding problems -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.Map;
import java.util.function.Consumer;
import software.amazon.smithy.utils.IoUtils;
import software.amazon.smithy.utils.MapUtils;

public final class IntegUtils {

Expand All @@ -47,7 +48,7 @@ public static void withProject(String projectName, Consumer<Path> consumer) {
}

public static void run(String projectName, List<String> args, Consumer<RunResult> consumer) {
run(projectName, args, Collections.emptyMap(), consumer);
run(projectName, args, MapUtils.of(EnvironmentVariable.NO_COLOR.toString(), "true"), consumer);
}

public static void run(String projectName, List<String> args, Map<String, String> env,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.not;

import org.junit.jupiter.api.Test;
import software.amazon.smithy.utils.ListUtils;
import software.amazon.smithy.utils.MapUtils;

public class RootCommandTest {
@Test
Expand All @@ -36,6 +38,9 @@ public void passing_h_printsHelp() {
IntegUtils.run("simple-config-sources", ListUtils.of("-h"), result -> {
assertThat(result.getExitCode(), equalTo(0));
ensureHelpOutput(result);

// We force NO_COLOR by default in the run method. Test that's honored here.
assertThat(result.getOutput(), not(containsString("[0m")));
});
}

Expand Down Expand Up @@ -71,6 +76,17 @@ public void errorsOnInvalidArgument() {
});
}

@Test
public void runsWithColors() {
IntegUtils.run("simple-config-sources",
ListUtils.of("--help"),
MapUtils.of(EnvironmentVariable.FORCE_COLOR.toString(), "true"),
result -> {
assertThat(result.getExitCode(), equalTo(0));
assertThat(result.getOutput(), containsString("[0m"));
});
}

private void ensureHelpOutput(RunResult result) {
// Make sure it's the help output.
assertThat(result.getOutput(),
Expand Down
150 changes: 150 additions & 0 deletions smithy-cli/src/main/java/software/amazon/smithy/cli/Ansi.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
/*
* Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

package software.amazon.smithy.cli;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.Objects;

/**
* Styles text using ANSI color codes.
*/
public enum Ansi {

/**
* Writes using ANSI colors if it detects that the environment supports color.
*/
AUTO {
private final Ansi delegate = Ansi.detect();

@Override
public String style(String text, Style... styles) {
return delegate.style(text, styles);
}

@Override
public void style(Appendable appendable, String text, Style... styles) {
delegate.style(appendable, text, styles);
}
},

/**
* Does not write any color.
*/
NO_COLOR {
@Override
public String style(String text, Style... styles) {
return text;
}

@Override
public void style(Appendable appendable, String text, Style... styles) {
try {
appendable.append(text);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}
},

/**
* Writes with ANSI colors.
*/
FORCE_COLOR {
@Override
public String style(String text, Style... styles) {
StringBuilder builder = new StringBuilder();
style(builder, text, styles);
return builder.toString();
}

@Override
public void style(Appendable appendable, String text, Style... styles) {
try {
appendable.append("\033[");
boolean isAfterFirst = false;
for (Style style : styles) {
if (isAfterFirst) {
appendable.append(';');
}
appendable.append(style.toString());
isAfterFirst = true;
}
appendable.append('m');
appendable.append(text);
appendable.append("\033[0m");
} catch (IOException e) {
throw new CliError("Error writing output", 2, e);
}
}
};

/**
* Detects if ANSI colors are supported and returns the appropriate Ansi enum variant.
*
* <p>This method differs from using the {@link Ansi#AUTO} variant directly because it will detect any changes
* to the environment that might enable or disable colors.
*
* @return Returns the detected ANSI color enum variant.
*/
public static Ansi detect() {
return isAnsiEnabled() ? FORCE_COLOR : NO_COLOR;
}

/**
* Styles text using ANSI color codes.
*
* @param text Text to style.
* @param styles Styles to apply.
* @return Returns the styled text.
*/
public abstract String style(String text, Style... styles);

/**
* Styles text using ANSI color codes and writes it to an Appendable.
*
* @param appendable Where to write styled text.
* @param text Text to write.
* @param styles Styles to apply.
*/
public abstract void style(Appendable appendable, String text, Style... styles);

private static boolean isAnsiEnabled() {
if (EnvironmentVariable.FORCE_COLOR.isSet()) {
return true;
}

// Disable colors if NO_COLOR is set to anything.
if (EnvironmentVariable.NO_COLOR.isSet()) {
return false;
}

String term = EnvironmentVariable.TERM.get();

// If term is set to "dumb", then don't use colors.
if (Objects.equals(term, "dumb")) {
return false;
}

// If TERM isn't set at all and Windows is detected, then don't use colors.
if (term == null && System.getProperty("os.name").contains("win")) {
return false;
}

// Disable colors if no console is associated.
return System.console() != null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,5 @@ default Consumer<String> testParameter(String name) {
*
* @param printer Printer to modify.
*/
default void registerHelp(HelpPrinter printer) {
// do nothing by default.
}
default void registerHelp(HelpPrinter printer) {}
}
72 changes: 48 additions & 24 deletions smithy-cli/src/main/java/software/amazon/smithy/cli/Cli.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

package software.amazon.smithy.cli;

import java.io.PrintWriter;
import java.io.StringWriter;
import java.util.Arrays;
import java.util.logging.Logger;

Expand All @@ -31,12 +33,8 @@ public final class Cli {

private static final Logger LOGGER = Logger.getLogger(Cli.class.getName());

// Delegate to the stdout consumer by default since this can change.
private CliPrinter stdoutPrinter = new CliPrinter.ConsumerPrinter(str -> System.out.print(str));

// Don't use a method reference in case System.err is changed after initialization.
private CliPrinter stdErrPrinter = new CliPrinter.ConsumerPrinter(str -> System.err.print(str));

private CliPrinter stdoutPrinter;
private CliPrinter stderrPrinter;
private final ClassLoader classLoader;
private final Command command;

Expand All @@ -63,37 +61,63 @@ public int run(String[] args) {
StandardOptions standardOptions = new StandardOptions();
arguments.addReceiver(standardOptions);

// Use or disable ANSI escapes in the printers.
CliPrinter out = new CliPrinter.ColorPrinter(stdoutPrinter, standardOptions);
CliPrinter err = new CliPrinter.ColorPrinter(stdErrPrinter, standardOptions);
if (stdoutPrinter == null || stderrPrinter == null) {
Ansi ansi = Ansi.detect();
if (stdoutPrinter == null) {
stdout(CliPrinter.fromOutputStream(ansi, System.out));
}
if (stderrPrinter == null) {
stderr(CliPrinter.fromOutputStream(ansi, System.err));
}
}

// Setup logging after parsing all arguments.
arguments.onComplete((opts, positional) -> {
LoggingUtil.configureLogging(opts.getReceiver(StandardOptions.class), err);
LoggingUtil.configureLogging(opts.getReceiver(StandardOptions.class), stderrPrinter);
LOGGER.fine(() -> "Running CLI command: " + Arrays.toString(args));
});

try {
return command.execute(arguments, new Command.Env(out, err, classLoader));
} catch (Exception e) {
err.printException(e, standardOptions.stackTrace());
throw CliError.wrap(e);
} finally {
try {
LoggingUtil.restoreLogging();
} catch (RuntimeException e) {
// Show the error, but don't fail the CLI since most invocations are one-time use.
err.println(err.style("Unable to restore logging to previous settings", Style.RED));
err.printException(e, standardOptions.stackTrace());
Command.Env env = new Command.Env(stdoutPrinter, stderrPrinter, classLoader);
return command.execute(arguments, env);
} catch (Exception e) {
printException(e, standardOptions.stackTrace());
throw CliError.wrap(e);
} finally {
try {
LoggingUtil.restoreLogging();
} catch (RuntimeException e) {
// Show the error, but don't fail the CLI since most invocations are one-time use.
printException(e, standardOptions.stackTrace());
}
}
} finally {
stdoutPrinter.flush();
stderrPrinter.flush();
}
}

public void stdout(CliPrinter printer) {
stdoutPrinter = printer;
public void stdout(CliPrinter stdoutPrinter) {
this.stdoutPrinter = stdoutPrinter;
}

public void stderr(CliPrinter stderrPrinter) {
this.stderrPrinter = stderrPrinter;
}

public void stderr(CliPrinter printer) {
stdErrPrinter = printer;
private void printException(Throwable e, boolean stacktrace) {
if (!stacktrace) {
stderrPrinter.println(e.getMessage(), Style.RED);
} else {
try (CliPrinter.Buffer buffer = stderrPrinter.buffer()) {
StringWriter writer = new StringWriter();
e.printStackTrace(new PrintWriter(writer));
String result = writer.toString();
int positionOfName = result.indexOf(':');
buffer.print(result.substring(0, positionOfName), Style.RED, Style.UNDERLINE);
buffer.println(result.substring(positionOfName));
}
}
}
}
Loading

0 comments on commit 8c52252

Please sign in to comment.