Skip to content

Pretty error reporting#696

Open
angelcerveraroldan wants to merge 1 commit into
coreos:mainfrom
angelcerveraroldan:butane-pretty-ux
Open

Pretty error reporting#696
angelcerveraroldan wants to merge 1 commit into
coreos:mainfrom
angelcerveraroldan:butane-pretty-ux

Conversation

@angelcerveraroldan
Copy link
Copy Markdown
Member

@angelcerveraroldan angelcerveraroldan commented Apr 14, 2026

The goal of this PR is to add pretty error reporting, since the difference is visual, I have attached screenshots for the errors that butane would report for the following butane:

variant: fcos
version: 1.6.0
boot_device:
  layout: s390x-virt
  mirror:
    devices:
      - /dev/vda
      - /dev/vdb
systemd:
  units:
    - name: example.service
      enabled: true
      contents: |
        [Unit]
        Description=Example
      contents_local: example.service
    - name: other.service
      dropinz:
        - name: override.conf
          contents: |
            [Service]
            ExecStart=
    - name: valid.service
      enabled: true

How errors show without this feature:

current-errors

How errors show with the new feature:

new-errors

New flags added:

  1. --pretty-errors to turn on pretty error reporting
  2. --color[=WHEN], where WHEN is auto (default), always, or never. This follows the same convention as ls, ripgrep, diff, bat, exa, ... In auto mode, colors are disabled if stderr is not a terminal or if the NO_COLOR environment variable is set. It also accepts colour just in case

This is an early PR to get feedback on whether or not this is a good addition to butane, or if it should go somewhere else / nowhere. I am not sure if the current flag name pretty-errors is good or not, seems kind of verbose to me.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new reporting package to provide enhanced, "pretty" error messages with source code context and ANSI colorization. The main entry point is updated to track input filenames and utilize these formatting options. Key feedback points include resolving a package name conflict that prevents compilation, implementing bounds checking for string slicing to avoid runtime panics, dynamically managing colorization for non-TTY outputs, and ensuring error underlines remain visible when pointing to whitespace.

Comment thread internal/report/report.go
Comment thread internal/report/report.go Outdated
Comment thread internal/main.go Outdated
Comment thread internal/report/report.go Outdated
@angelcerveraroldan angelcerveraroldan force-pushed the butane-pretty-ux branch 2 times, most recently from 251c107 to 01ef686 Compare May 5, 2026 14:58
@angelcerveraroldan angelcerveraroldan force-pushed the butane-pretty-ux branch 2 times, most recently from 9649621 to 474396f Compare May 7, 2026 16:07
@angelcerveraroldan angelcerveraroldan marked this pull request as ready for review May 7, 2026 16:50
@angelcerveraroldan angelcerveraroldan changed the title WIP: Pretty error reporting Pretty error reporting May 8, 2026
Add an optional flag to to enable pretty rust-like error reporting for
butane errors.

This introduces the flags:
- pretty-errors		(auto, always, or never)
- color / colour	(auto, always, or never)

While also respecting the `NO_COLOR` environment variable.
Copy link
Copy Markdown
Member

@travier travier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me. +1 from me. Untested.

@travier
Copy link
Copy Markdown
Member

travier commented May 27, 2026

--pretty-errors to turn on pretty error reporting

Should we have this on by default? Butane output should be for humans so good default for humans sounds like a good idea.

@angelcerveraroldan
Copy link
Copy Markdown
Member Author

--pretty-errors to turn on pretty error reporting

Should we have this on by default? Butane output should be for humans so good default for humans sounds like a > good idea.

I did not enable it by default in case the raw output was being used in any automatic things. But if its meant for humans only, then having it by default may be nice, as otherwise a lot of people may not know it is available. Open to going either way.

Comment thread internal/main.go
pflag.BoolVarP(&strict, "strict", "s", false, "fail on any warning")
pflag.BoolVarP(&options.Pretty, "pretty", "p", false, "output formatted json")
pflag.BoolVarP(&options.Raw, "raw", "r", false, "never wrap in a MachineConfig; force Ignition output")
pflag.BoolVar(&prettyErrors, "pretty-errors", false, "pretty print errors")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I think opt in makes sesne, but having the many different flags can lead to a little confusion. separate flags imply they do something separately, i.e --color without --pretty-errors does not really do anything right?

Is there a way we can make --color a sub-option of --pretty-errors?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I had setup color with those flags is to follow the standard convention that most programs use.

I do think that having colors only be used for pretty errors can be strange, specially so if pretty errors is opt-in rather than opt-out. I am not entirely sure how to make colors a sub option of pretty errors nicely.

Maybe something like: --pretty-errors=color, --pretty-errors=no-color, as well as --pretty-errors=auto, where auto checks if its a tty or not / if NO_COLOR is set. (If --pretty-errors is not set then use the normal errors)

I am not sure if this is less confusing or not.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would default to the new pretty output and add another option (--json ?) to output machine readable output instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants