apple / pkl

A configuration as code language with rich validation and tooling.
https://pkl-lang.org
Apache License 2.0
9.84k stars 259 forks source link

Add colours to Pkl errors in Cli output #552

Closed thomaspurchas closed 1 day ago

thomaspurchas commented 6 days ago

To make error messages from Pkl eval easier to read, this change uses the Jansi library to colour the output, making it quicker and easier to scan error messages and understand what's happened.

The Jansi library also detects if the CLI output is a terminal capable of handling colours, and will automatically strip out escape codes if the output won't support them (e.g. piping the output somewhere else).

The implementation is a little crude, but think acceptable given a better approach would probably require rewriting how errors are handled completely, so that the output composition happens near the frontend of the CLI, rather than down in the bowls of Pkl. The CLI frontend is in a much better position to determine terminal capabilities, and adjust outputs as needed, and would allow us to use a much more expressive library like Mordant to really supercharge the output formatting.

image

image image
thomaspurchas commented 5 days ago

@holzensp that makes sense. I'll try and find a better way to disabling the colour output during tests, ideally it would be disabled by default (and without the need for the removeColors helper), except for tests looking to test the colour output, but I haven't managed to nail down a good approach yet.

thomaspurchas commented 5 days ago

@holzensp I've rewritten how tests are handled, and managed to configure the test environment to complete remove the need for any special handling of colours in normal tests, and I've added a some tests that specifically test the colour outputs.

thomaspurchas commented 4 days ago

Also, we currently only do native-image builds on CI in prerelease; have you tested building native images locally?

Yes I have, it took a few tries to figure out how to make everything work correctly in native images. Thankfully the Jansi library comes with pre-built native libraries (needed for low level syscalls to interrogate the terminal capabilities) for all major OSs and Arch's.

I only needed to make a very small tweak to the CLI build config to get all of this working properly in both jpkl and the native images. Admittedly I've only tested MacOS / arm64, but I don't see any reason why other OS's or Arch's should have any issues.

bioball commented 4 days ago

This looks great! Thanks for the PR!

With outputting colors in our console, I think we will also want a --color flag, where it accepts "never", "always", and "auto" (auto means: output colors if stdout/stderr are TTY).

a better approach would probably require rewriting how errors are handled completely, so that the output composition happens near the frontend of the CLI, rather than down in the bowls of Pkl. The CLI frontend is in a much better position to determine terminal capabilities, and adjust outputs as needed, and would allow us to use a much more expressive library like Mordant to really supercharge the output formatting.

I don't think we really need a whole revamp of error messages here. Rather, the CLI can pass a text formatter into the APIs within pkl-core. This is already a pattern that we use (see org.pkl.core.StackFrameTransformer).

Also: it would be good to avoid adding jansi as a dependency in pkl-core. This would be another benefit of providing a "text formatter" interface, of which we can have an implementation that outputs Ansi colors (defined in pkl-commons-cli), and an implementation that strips colors (defiend in pkl-core).

These comments don't necessarily need to block the PR, but we'll probably want to address this before we ship 0.27.

Color suggestion: the red error message seems a little harsh to me. How about making it bold? Something like this (here, both "Pkl Error" and the message are bold)

Screenshot 2024-06-28 at 4 41 05 PM
thomaspurchas commented 3 days ago

I don't think we really need a whole revamp of error messages here. Rather, the CLI can pass a text formatter into the APIs within pkl-core. This is already a pattern that we use (see org.pkl.core.StackFrameTransformer).

Happy to explore those options once we've got this merged. Agree it would be good to avoid the Jansi dependence in Pkl-core and find a better way to cleanly separate these concerns. I do also want to look at whole error generation and formatting process, allowing errors to propagate more context that the formatting process can take advantage, so colour hints etc can be embedded into the error messages themselves.

Color suggestion: the red error message seems a little harsh to me. How about making it bold? Something like this (here, both "Pkl Error" and the message are bold)

We can adjust this, but it's worth remembering that the exact colours used here are determined by your terminal not by Pkl. So the "harshness" will depend quite a bit on the terminal config, so there not a clearcut good set of colour choices. I think the red matches peoples expectations of what error messages look like, so I don't think it worth changing, and it's aways easy to change it in the future based on feedback we get.

image image image image
bioball commented 2 days ago

Thanks for the additional screenshots. Albeit, I still feel that it's a little harsh in each of those screen grabs.