Add a --color flag to the CLI and disable colors for Pkl test

[thomaspurchas]

The --colors flag makes it possible to forcefully disable colour output, and also forcefully enable it. We make use of this in the native Pkl test engines by passing --colors=never into the pkl commands, which ensures we get a predictable output.

Additionally the pkl test command also sets --colors=never to ensure that pkl test output isn't accidentially contaminated with ansi escape chars, which will break example based tests.

[bioball]

Some minor comments!

Also, this is missing documentation (cli-common-options.adoc, gradle-common-properties.adoc). Do you mind adding them?

[bioball]

Let's make this an enum:

enum class Color {
  /**
   * Output ANSI-formatted text only when the process STDOUT/STDERR are attached to a TTY.
   */
  AUTO,

  /**
   * Never output ANSI-formatted text.
   */
  NEVER,

  /**
   * Always output ANSI-formatted text.
   */
  ALWAYS
}

And change this type to said enum, and also add a doc comment. Also, nit: s/colors/color

Suggested change

	val colors: String = "auto"
	/** Whether to output ANSI-formatted text. */
	val color: Color = Color.AUTO

[bioball]

Let's follow the convention set by unix-y tools like ls, diff, grep, etc, and call this --color.

Also, with the above suggestion to enum-ify this (requires import com.github.ajalt.clikt.parameters.types.enum)

Suggested change

	val colors: String by
	option(names = arrayOf("--colors"), help = "Enable or disable colour output in the terminal")
	.choice("auto", "never", "always")
	.default("auto")
	val color: Color by
	option(names = arrayOf("--color"), help = "Enable or disable colour output in the terminal")
	.enum<Color>(key = { it.name.lowercase() })
	.default(Color.AUTO)

[bioball]

Suggested change

	colors = if (disableColors) "never" else colors,
	color = color

[thomaspurchas]

This exists to provide a mechanism for the test subcommand to forcefully disable not just colours, but also the injection if ANSI codes completely. I've been having issues because the pkl test expected output because they don't travel along the standard output rails. As a consequence the ANSI codes don't get stripped if they're injected.

By providing this escape hatch, I can ensure that colour are disabled before there's any opportunity for the Jansi formatting object to init, because once it's created you can't disable the injection of ANSI codes, only strip them out later. That mixed with the fact that caught errors in pkl test are truncated creates a bit of nightmare as the outputs get truncated to slightly different lengths depending on if colours were enabled, regardless of if those ANSI code actually made it to the output.

I tried to figure out where the error truncation etc was happening, so I could modify it to handle ANSI/non-ANSI outputs in an equivalent way, but I haven't been able to track down where it happens.

[bioball]

I think if (when?) we switch to a text formatter, we can control this ourselves by passing a no-op text formatter when --color=never. Then, we're in full control of how formatting happens rather than mutating global state.

Anyways, I don't know if that's relevant to my suggestion here. CliBaseOptions is a data class, and you can always modify it however you want because you get a derived copy method.

val options = baseOptions(myModules).copy(color = Color.NEVER)

[thomaspurchas]

It's only relevant due to how Jansi handles global state. It's caught me out a couple of times because Jansi doesn't handle changes to global state the way you might expect. The final result is what you expect, but the mechanism by which Jansi achieves that result depends on the exact order of operations (mutating global state before or after doing the initial Jansi setup). In most cases it doesn't make any practical difference, but for the specific case of Pkl test it does (which I learned the hard way).

For this specific case I need to make sure the baseOptions are set correctly before the common CLI setup function runs, because that's where Jansi setup happens. It's not enough to only modify the state within the execution context of the test command (which is what I originally tried).

Agree we can just get rid of this hack by implementing a proper formatting API, and having direct control over how control codes are injected. It's also a good argument to do a proper formatting API sooner rather than later. The number of hacks and shortcuts I've had to introduce to get Jansi working as expected is getting pretty uncomfortable at this point. So I'll probably look towards doing that, rather than continuing further with this approach.

[bioball]

Suggested change

disableColors: Boolean = false,

[bioball]

I'm pretty sure this means that Gradle will not output any colors, because this is executed as a child process.

Anyway, we should provide the same options in Gradle (auto, never, always). Take a look at how the other options are configured to see what to do here. Or, leave a TODO comment here so we can follow up in a future PR.

[holzensp]

This may be more complicated than what it's worth, but really, pkl test should color the "outside" exceptions and not the "inside" ones, i.e. if pkl test itself throws, it should be colored, but anything inside a module.catch(...) should not.

I don't think this is something that should hold you back, especially considering we want to try and avoid the Jansi dependency in pkl-core. @bioball's suggestion of passing in a formatter does seem the most sensible way to go.

[thomaspurchas]

@holzensp yeah my plan to stop working on this approach, and look at passing in a formatter. As I mentioned here, the number of hacks and whatnot that are piling up to make this approach work doesn't feel good. I wouldn't be comfortable merging such a fragile pile of hacks and workarounds.