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

CLI-18: customize error messages for test command #115

Merged
merged 4 commits into from
Feb 27, 2021
Merged

Conversation

morganelle
Copy link
Contributor

Description

After talking with @Widcket, our error style guide is writing custom error messages for errors with known causes and prefixing and passing through unknown errors.

This PR takes a pass at error messages for the test command.

References

https://auth0team.atlassian.net/wiki/spaces/~474568858/pages/1675758375/Auth0+CLI+DX+Proposal#Keep-error-messages-user-focused
https://auth0team.atlassian.net/jira/software/projects/CLI/boards/1063?selectedIssue=CLI-18

Testing

Tested manually

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@morganelle morganelle requested a review from Widcket February 26, 2021 22:27
@@ -46,14 +46,14 @@ Launch a browser to try out your universal login box for the given client.
if clientID == "" {
client, err := getOrCreateCLITesterClient(cli.api.Client)
if err != nil {
return err
return fmt.Errorf("Unable to test the login box; please check your internet connection and verify you haven't reached your apps limit")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("Unable to test the login box; please check your internet connection and verify you haven't reached your apps limit")
return errors.New("Unable to test the login box; please check your internet connection and verify you haven't reached your apps limit")

Given you're not passing %s style args, can just errors.New it.

Copy link

Choose a reason for hiding this comment

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

can we identify if the error is "reach app limit" vs internet issue? I would want to see the underlying error message, unless we know the two type of things that can happen are internet issue or app limit...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Updated to Unable to create an app for testing the login box: {error} and Unable to create an app to test getting a token: {error}.

Copy link
Contributor

@cyx cyx left a comment

Choose a reason for hiding this comment

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

Looks good! Just that small nit on fmt.Errorf use vs errors.New.

@morganelle
Copy link
Contributor Author

Looks good! Just that small nit on fmt.Errorf use vs errors.New.

Ah thanks! Updated

@Widcket Widcket merged commit c60218c into main Feb 27, 2021
@Widcket Widcket deleted the error-messages branch February 27, 2021 00:11
@Widcket
Copy link
Contributor

Widcket commented Feb 27, 2021

@morganelle thanks!

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.

4 participants