Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix: show --help message for CLI errors, add tests for delete#1403

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

jsjoeio
Copy link
Contributor

@jsjoeiojsjoeio commentedMay 11, 2022•
edited
Loading

This PR shows a more helpful error message when callingcoder delete.

Subtasks

  • added tests

Fixes#1233

H/T to@mafredri for pointers & suggestions 🎉

First time actually working in Go so please give any and all suggestions

@codecov
Copy link

codecovbot commentedMay 11, 2022•
edited
Loading

Codecov Report

Merging#1403 (8a5fc02) intomain (f4da5d4) willincrease coverage by0.09%.
The diff coverage is100.00%.

@@            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
FlagCoverage Δ
unittest-go-macos-latest54.24% <100.00%> (+0.03%)⬆️
unittest-go-postgres-65.72% <100.00%> (+0.04%)⬆️
unittest-go-ubuntu-latest56.59% <100.00%> (+0.10%)⬆️
unittest-go-windows-202252.68% <100.00%> (+0.05%)⬆️
unittest-js74.73% <ø> (+0.49%)⬆️
Impacted FilesCoverage Δ
cli/cliarg/cliarg.go100.00% <100.00%> (ø)
cli/delete.go57.14% <100.00%> (+32.14%)⬆️
coderd/roles.go68.96% <0.00%> (-31.04%)⬇️
codersdk/roles.go63.63% <0.00%> (-9.10%)⬇️
cli/autostart.go66.37% <0.00%> (-8.92%)⬇️
cli/autostop.go66.37% <0.00%> (-8.63%)⬇️
coderd/httpapi/httpapi.go71.25% <0.00%> (-6.25%)⬇️
site/src/components/Section/Section.tsx61.11% <0.00%> (-5.56%)⬇️
site/src/xServices/auth/authXService.ts78.12% <0.00%> (-4.49%)⬇️
cli/ssh.go39.53% <0.00%> (-1.24%)⬇️
... and47 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatef4da5d4...8a5fc02. Read thecomment docs.

Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

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

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 0Delete a workspaceFlags:  -h, --help   help for deleteUse "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! TheUse: description is not visible so it's impossible to know what the argument is 🤔.

jsjoeio reacted with eyes emoji
@jsjoeiojsjoeioforce-pushed the1233-bug-ux-coder-workspaces-delete-error-message branch fromd09bb5e toba2f9b7CompareMay 12, 2022 22:20
@jsjoeiojsjoeio self-assigned thisMay 12, 2022
@jsjoeiojsjoeio requested review froma team andmafredriMay 12, 2022 22:36
@jsjoeiojsjoeio marked this pull request as ready for reviewMay 12, 2022 22:37
Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

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

I noticed a limitation of our custom template for command help that may have contributed to part of the usability problem with the CLI. I think we should get that fixed first to better see what's missing.

@jsjoeio
Copy link
ContributorAuthor

I noticed a limitation of our custom template for command help that may have contributed to part of the usability problem with the CLI. I think we should get that fixed first to better see what's missing.

Nice find! Even if we fix that, I believe this original issue will still exist. Try runninggh pr close and you'll see they have the same issue.

What if we:

  • address changes in this PR and merge
  • tackle the template issue in a separate PR
    ?
dwahler and mafredri reacted with thumbs up emoji

@greyscaledgreyscaled changed the titlefix(cli): show error message for [delete] without parameterfix: show error message for [delete] without parameterMay 14, 2022
mafredri added a commit that referenced this pull requestMay 16, 2022
@mafredri
Copy link
Member

Good idea to split into separate PR@jsjoeio, I created the PR for fixing the--help output in#1463.

jsjoeio reacted with hooray emoji

@missknissmisskniss added this to theCommunity MVP milestoneMay 16, 2022
@jsjoeiojsjoeio marked this pull request as draftMay 19, 2022 16:06
This adds a new test for the `delete` command to ensure it works asexpected when provided the correct args.
@jsjoeiojsjoeioforce-pushed the1233-bug-ux-coder-workspaces-delete-error-message branch from8a5fc02 to758dcebCompareMay 19, 2022 16:13
This modifies the `cli.Root().Execute()` to `cli.Root).ExecuteC()` tomatch the default behavior of Cobra. We do this so errors will alwaysprint the "run --help" line.
This adds a new test to the `delete_test.go` suite to ensure the correctbehavior occurs when `delete` is called without an argument.
This adds an error message which shows when there is an error with anycommands called to improve the UX.
Comment on lines 22 to 23
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))
Copy link
Member

Choose a reason for hiding this comment

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

Most errors are not usage errors and shouldn't show the usage. Stuff like HTTP errors would trigger this for example

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

True!@f0ssel mentioned the same thing. Maybe that's whySilenceErrors is set totrue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can detect a cobra error likeerrors.Is(err, cobra.ErrArgs) or something?

jsjoeio reacted with eyes emoji
Copy link
ContributorAuthor

@jsjoeiojsjoeioMay 19, 2022•
edited
Loading

Choose a reason for hiding this comment

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

I looked around the docs but didn't find anything. Seems like it just returns aregular 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 errorslike 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)
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Note: this behavior mimics whatSilenceErrors: 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 {
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Extracted into function to make it easier to test

@jsjoeiojsjoeio marked this pull request as ready for reviewMay 19, 2022 19:16
@jsjoeiojsjoeio changed the titlefix: show error message for [delete] without parameterfix: show --help message for CLI errors, add tests for deleteMay 19, 2022
pty.ExpectMatch("Cleaning Up")
<-doneChan
})
t.Run("WithoutParameters/FormatCobraError", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: nothing wrong with this test case that I can see, but since it's just testing theFormatCobraError function and doesn't have anything to do with the functionality ofcoder delete, it seems a bit odd for it to live in this file. How about creatingroot_test.go and putting it there?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Good point!@f0ssel and I were discussing that as well and we had landed on leaving here but happy to move it!

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Done

@jsjoeiojsjoeioenabled auto-merge (squash)May 19, 2022 22:25
@jsjoeiojsjoeio merged commit6dae48a intomainMay 19, 2022
@jsjoeiojsjoeio deleted the 1233-bug-ux-coder-workspaces-delete-error-message branchMay 19, 2022 22:36
kylecarbs pushed a commit that referenced this pull requestJun 10, 2022
* feat(cli): add test for deleteThis adds a new test for the `delete` command to ensure it works asexpected when provided the correct args.* fix(cli): use ExecuteC() to match CobraThis modifies the `cli.Root().Execute()` to `cli.Root).ExecuteC()` tomatch the default behavior of Cobra. We do this so errors will alwaysprint the "run --help" line.* feat(cli): add WithoutParameters test for deleteThis adds a new test to the `delete_test.go` suite to ensure the correctbehavior occurs when `delete` is called without an argument.* fixup! feat(cli): add WithoutParameters test for delete* refactor(cli): show --help error message on mainThis adds an error message which shows when there is an error with anycommands called to improve the UX.* fixup! refactor(cli): show --help error message on main* refactor(cli): handle err with FormatCobraErrorThis adds a new helper function called `FormatCobraError` to `root.go`so that we can colorize and add "--help" message to cobra command errorslike calling `delete`.* refactor(cli): add root_test.go, move delete test
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@EmyrkEmyrkEmyrk left review comments

@dwahlerdwahlerdwahler approved these changes

@mafredrimafredriAwaiting requested review from mafredri

@deansheatherdeansheatherAwaiting requested review from deansheather

@f0sself0sselAwaiting requested review from f0ssel

Assignees

@jsjoeiojsjoeio

Labels
None yet
Projects
None yet
Milestone
Community MVP
Development

Successfully merging this pull request may close these issues.

Bug (UX): ./coder workspaces delete -> error message
7 participants
@jsjoeio@mafredri@dwahler@Emyrk@deansheather@f0ssel@misskniss
[8]ページ先頭
©2009-2025 Movatter.jp