Refactor CLI printing and colors #1525
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mtdowling
force-pushed
the
cli-absolute-paths
branch
2 times, most recently
from
50e720e to
17c1a2f
Compare
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.
mtdowling
force-pushed
the
cli-printer-refactor
branch
from
5b32e87 to
fbb1f61
Compare
JordonPhillips
requested changes
Dec 7, 2022
Per request, adding back support for these arguments. This was achieved by separating the ColorFormatter from the CliPrinter. Because CLI arguments aren't fully parsed when bootstrapping the CLI and there needs to be a resolved ColorFormatter to create an Environment to give to a comment, I added a ColorFormatter interface that is implemented by AnsiColorFormatter (formally Ansi). The CLI will create an instance of a ColorFormatter that defers whether color is supported or not by creating a custom ColorFormatter that queries StandardOptions#colorSetting before each write.
mtdowling
force-pushed
the
cli-printer-refactor
branch
from
a3bbdd8 to
d521dab
Compare
JordonPhillips
approved these changes
Dec 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 also done through environment variables (FORCE_COLOR, NO_COLOR). This matches lots of other CLI tools.
The way in which colors are detected and rendered is updated too. This change introduces ColorFormatter, and 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. The CLI now passes around a ColorFormatter in addition to stdout/stderr CliPrinter instances. ColorFormatter could potentially be swapped out for other implementations too (like a markdown renderer).
The hueristic used to detect colors is now:
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.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.