Skip to content
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

feat: Add more help in error message #72

Merged
merged 4 commits into from
Mar 27, 2024
Merged

Conversation

dbolson
Copy link
Contributor

@dbolson dbolson commented Mar 26, 2024

When a command doesn't have the right flags, we show a one line error message that describes the error and says how to see more info with --help instead of showing the results of calling --help. This can cut down on noise while still giving the user enough info for how to fix the error.

If the error comes from the API, just show that error like normal.

The "flag needs an argument" message doesn't show the additional help text because it's more difficult to hook into the pflags error where it originates instead of the other errors that come from cobra.

$ ldcli flags update
=> required flag(s) "accessToken", "data", "key", "projKey" not set. See `ldcli flags update --help` for supported flags and usage.
$ ldcli flags update -u invalidBaseUri
=> baseUri is invalid. See `ldcli flags update --help` for supported flags and usage.
$ flags update -t api-1fe1f428-bf2e-453e-9790-4a260fdf3391 -u http://localhost:3000 -d '[{"op": "replace", "path": "/environments/production/on", "value": false}]' --key cli-flag2 --projKey
=> flag needs an argument: --projKey
$ flags update -t api-1fe1f428-bf2e-453e-9790-4a260fdf3391 -u http://localhost:3000 -d '[{"op": "replace", "path": "/environments/production/on", "value": false}]' --key cli-flag2 --projKey foo
=> {"message":"Unknown project key foo"}

Copy link

This pull request has been linked to Shortcut Story #237653: show the right usage help on subcommand error.

"ldcli/internal/flags"
)

func NewCreateCmd(client flags.Client) (*cobra.Command, error) {
cmd := &cobra.Command{
Use: "create",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorted and moved the validate function into validators.Validate.

@@ -12,6 +12,7 @@ import (
)

func TestCreate(t *testing.T) {
errorHelp := ". See `ldcli flags create --help` for supported flags and usage."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can refactor these tests to DRY up some duplication.


// Validate is a validator for commands to print an error when the number provided
// is different than the arguments passed in
func Validate() cobra.PositionalArgs {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly taken from stripe's CLI.

return func(cmd *cobra.Command, args []string) error {
commandPath := getCommandPath(cmd)

_, err := url.ParseRequestURI(viper.GetString("baseUri"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may want to extract unexported functions if this grows.

Comment on lines 14 to 15
// Validate is a validator for commands to print an error when the number provided
// is different than the arguments passed in
Copy link
Contributor

@k3llymariee k3llymariee Mar 26, 2024

Choose a reason for hiding this comment

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

can we update this docstring since we're doing more than just validating the # of args (or just remove)

@dbolson dbolson merged commit 6221983 into main Mar 27, 2024
1 of 2 checks passed
@dbolson dbolson deleted the sc-237653/fix-subcommand-errors branch March 27, 2024 18:39
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