-
Notifications
You must be signed in to change notification settings - Fork 609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: change cli error message handling #9952
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
||
func errorWithStackTrace() error { | ||
return xerrors.Errorf("function decided not to work, and it never will") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Re: lines 103 to 103]
Nifty
See this comment inline on Graphite.
} | ||
return str.String() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is the last remnant of lipgloss
in our codebase. If you could edit the code to use the pretty
style that would be a nice side-win for the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL, can do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lipgloss is still in the go.mod file because of slogtest:
https://github.com/coder/slog/blob/main/internal/entryhuman/entry.go#L19
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. This looks awesome.
What this does
This changes how we format cli error messages, and adds a hidden command to easily get these errors on the UI. I was trying to run commands with bad args to test the visuals, and it was annoying.
Basically, instead of trying to handle errors as a general construct, I just wrote a formatter for each error type we expect to encounter. If we add more sentinel errors, we can just write a custom formatter here. It is easier then trying to be "smart" and general.
I am writing these all in 1 spot rather than making some
CliError() string
method to share styles and keep it all together. I think we will modify this more as we tailor the look of things.The biggest thing here is that verbosity is now a flag passed to the formatter.
Closes:
#9492
#9577
Error examples
Multi-Errors
These would be very rare. Before on left, after on right.
CMD Errors
Very common. All cmd errors are wrapped as RunCommandErrors.
API Errors
Missing required flag
Flag validation
This one sucks. The error from pflag is opaque: https://github.com/spf13/pflag/blob/master/flag.go#L478
😢 The package is very unmaintained. Might be worth forking and adding some better errors for us.
There is some conversation on the maintenance of it, but who knows what will come of that.
spf13/pflag#385
Issue that is related to what we want here: spf13/pflag#269
https://github.com/spf13/pflag/pulls?q=is%3Apr+is%3Aclosed