fix: show --help message for CLI errors, add tests for delete #1403
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1403 +/- ##
==========================================
+ Coverage 67.06% 67.15% +0.09%
==========================================
Files 288 292 +4
Lines 19079 19533 +454
Branches 241 258 +17
==========================================
+ Hits 12795 13118 +323
- Misses 4978 5063 +85
- Partials 1306 1352 +46
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Added some thoughts, in case they're helpful. I love that you're looking to fix this, it's not the best experience to have to dig into --help every time you've done something "wrong".
Perhaps an even better option than custom argument processing would be to automatically show the command help on error?
accepts 1 arg(s), received 0
Delete a workspace
Flags:
-h, --help help for delete
Use "coder delete [command] --help" for more information about a command.
The only problem I noticed here is that the --help is not helpful at all! The Use: description is not visible so it's impossible to know what the argument is 🤔.
d09bb5e to
ba2f9b7
Compare
Nice find! Even if we fix that, I believe this original issue will still exist. Try running What if we:
|
This adds a new test for the `delete` command to ensure it works as expected when provided the correct args.
8a5fc02 to
758dceb
Compare
This modifies the `cli.Root().Execute()` to `cli.Root).ExecuteC()` to match the default behavior of Cobra. We do this so errors will always print the "run --help" line.
This adds a new test to the `delete_test.go` suite to ensure the correct behavior occurs when `delete` is called without an argument.
This adds an error message which shows when there is an error with any commands called to improve the UX.
cmd/coder/main.go
Outdated
| helpErrMsg := fmt.Sprintf("Run '%s %s --help' for usage.", cmd.Root().Name(), cmd.Name()) | ||
| _, _ = fmt.Fprintln(os.Stderr, cliui.Styles.Error.Render(err.Error()+"/n"+helpErrMsg)) |
There was a problem hiding this comment.
Most errors are not usage errors and shouldn't show the usage. Stuff like HTTP errors would trigger this for example
There was a problem hiding this comment.
True! @f0ssel mentioned the same thing. Maybe that's why SilenceErrors is set to true?
There was a problem hiding this comment.
Is there a way we can detect a cobra error like errors.Is(err, cobra.ErrArgs) or something?
There was a problem hiding this comment.
I looked around the docs but didn't find anything. Seems like it just returns a regular error. Maybe someone sees something though.
This adds a new helper function called `FormatCobraError` to `root.go` so that we can colorize and add "--help" message to cobra command errors like calling `delete`.
| if err != nil { | ||
| if errors.Is(err, cliui.Canceled) { | ||
| os.Exit(1) | ||
| } | ||
| _, _ = fmt.Fprintln(os.Stderr, cliui.Styles.Error.Render(err.Error())) | ||
| cobraErr := cli.FormatCobraError(err, cmd) |
There was a problem hiding this comment.
Note: this behavior mimics what SilenceErrors: true does, which prints the --help message below any errors. See this comment from @mafredri for more context: #1403 (comment)
| @@ -259,3 +260,9 @@ func versionTemplate() string { | |||
| template += "\r\n" | |||
| return template | |||
| } | |||
|
|
|||
| // FormatCobraError colorizes and adds "--help" docs to cobra commands. | |||
| func FormatCobraError(err error, cmd *cobra.Command) string { | |||
There was a problem hiding this comment.
Extracted into function to make it easier to test
cli/delete_test.go
Outdated
| pty.ExpectMatch("Cleaning Up") | ||
| <-doneChan | ||
| }) | ||
| t.Run("WithoutParameters/FormatCobraError", func(t *testing.T) { |
There was a problem hiding this comment.
Nitpick: nothing wrong with this test case that I can see, but since it's just testing the FormatCobraError function and doesn't have anything to do with the functionality of coder delete, it seems a bit odd for it to live in this file. How about creating root_test.go and putting it there?
There was a problem hiding this comment.
Good point! @f0ssel and I were discussing that as well and we had landed on leaving here but happy to move it!
* feat(cli): add test for delete This adds a new test for the `delete` command to ensure it works as expected when provided the correct args. * fix(cli): use ExecuteC() to match Cobra This modifies the `cli.Root().Execute()` to `cli.Root).ExecuteC()` to match the default behavior of Cobra. We do this so errors will always print the "run --help" line. * feat(cli): add WithoutParameters test for delete This adds a new test to the `delete_test.go` suite to ensure the correct behavior occurs when `delete` is called without an argument. * fixup! feat(cli): add WithoutParameters test for delete * refactor(cli): show --help error message on main This adds an error message which shows when there is an error with any commands called to improve the UX. * fixup! refactor(cli): show --help error message on main * refactor(cli): handle err with FormatCobraError This adds a new helper function called `FormatCobraError` to `root.go` so that we can colorize and add "--help" message to cobra command errors like calling `delete`. * refactor(cli): add root_test.go, move delete test
This PR shows a more helpful error message when calling
coder delete.Subtasks
Fixes #1233
H/T to @mafredri for pointers & suggestions 🎉
First time actually working in Go so please give any and all suggestions