-
Notifications
You must be signed in to change notification settings - Fork 36
print usage only on parse/validation errors #493
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
Conversation
879d932 to
8adc5ab
Compare
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.
Could you post examples of how the CLI looks before & after this PR? I understand:
Before this PR:
$ zed preview schema compile root.zed
Usage:
zed preview schema compile <file> [flags]
Examples:
Write to stdout:
zed preview schema compile root.zed
Write to an output file:
zed preview schema compile --out compiled.zed
Flags:
-h, --help help for compile
--out string output filepath; omitting writes to stdout
Global Flags:
--certificate-path string path to certificate authority used to verify secure connections
--endpoint string spicedb gRPC API endpoint
--hostname-override string override the hostname used in the connection to the endpoint
--insecure connect over a plaintext connection
--log-format string format of logs ("auto", "console", "json") (default "auto")
--log-level string verbosity of logging ("trace", "debug", "info", "warn", "error") (default "info")
--max-message-size int maximum size *in bytes* (defaults to 4_194_304 bytes ~= 4MB) of a gRPC message that can be sent or received by zed
--no-verify-ca do not attempt to verify the server's certificate chain and host name
--permissions-system string permissions system to query
--request-id string optional id to send along with SpiceDB requests for tracing
--skip-version-check if true, no version check is performed against the server
--token string token used to authenticate to SpiceDB
3:29PM ERR terminated with errors error="failed to read schema file: open root.zed: no such file or directory"
883d20e to
d1514db
Compare
|
I love the general idea of this, unfortunately I think this makes the UX a little worse in some other cases.
Here's a common one I run into with zed (because I can never remember the Here's before this PR: and after: Note that prior to this PR it would give me the info I need - the format to put a check query. |
d1514db to
11031ff
Compare
the usage was being printed on any error, which was rather unpleasant to look at because it makes the output difficult to parse for clues on what happened. The whole goal of this commit is to improve UX. Cobra supports "SilenceErrors" and "SilenceUsage", and a callback function "SetFlagErrorFunc" for flag parsing errors. Unfortunately, only a subset of errors fall under "flag parsing error" category. To amend this, a special error Wrapper is introduced that signals an error is a validation error, so the command can manually print the usage when "SilenceUsage" is enabled, which this commit does. A few tests are added that assert the output. Due to the pervasive use of globals, some refactoring was conducted in cmd.go to make it more test friendly.
11031ff to
b63f5e1
Compare
ecordell
left a comment
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.
LGTM
Built on top of #492
The usage was being printed on any error, which was rather unpleasant to look at because it made the output difficult to parse for clues on what happened. The primary goal of this commit is to enhance the user experience.
Cobra supports
SilenceErrorsandSilenceUsage, as well as a callback function,SetFlagErrorFunc, for handling flag parsing errors and controlling when usage and errors are printed. Unfortunately, only a subset of errors fall under the "flag parsing error" category: validation errors defined viaCommand.Argsdo not triggerFlagErrorFunc, so ifSilenceUsageis enabled, we would miss the usage for those validation cases 😢To address this, a special error wrapper is introduced that signals an error is a validation error, allowing the command to manually print the usage when
SilenceUsageis enabled, which this commit does.A few tests are added that assert the output. Due to the pervasive use of globals, some refactoring was conducted in
cmd.goto make it more test-friendly.This is what the command line output looked like when a normal application error happened:
This is what it looks like now after the change