Skip to content

Better error messages when you don't supply an arg#144

Merged
probablycorey merged 9 commits intomasterfrom
better-better-better
Dec 12, 2019
Merged

Better error messages when you don't supply an arg#144
probablycorey merged 9 commits intomasterfrom
better-better-better

Conversation

@probablycorey
Copy link
Copy Markdown
Contributor

@probablycorey probablycorey commented Dec 11, 2019

Closes #75

gh pr checkout

From requires at least 1 arg(s), only received 0 to requires a PR number as an argument

gh pr view

From no open pull requests found for branch “empty-pr-list” to no open pull requests found for branch “empty-pr-list”. To open a specific PR use a PR number as an argument

gh issue view
From requires at least 1 arg(s), only received 0 to requires an issue number as an argument

@probablycorey probablycorey self-assigned this Dec 11, 2019
command/pr.go Outdated
if err != nil {
return err
}
return fmt.Errorf("no open pull requests found for branch %q. To open a specific PR use a PR number as an argument", branch)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could keep the old behavior and detect a particular error from PullRequestForBranch and add extra error text:

if err != nil {
  if strings.Contains(err.Error(), "no open pull requests found") {
    err = fmt.Errorf("%s\nTo open a specific pull request, add the pull request number as argument", err)
  }
  return err
}

func (e *NotFoundError) Error() string {
return e.message
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can probably use an error like this in situations where no issue or pr is found, like gh pr view 239874987. But I left that for another PR.

@probablycorey
Copy link
Copy Markdown
Contributor Author

I gave this a go with the custom Error struct idea, let me know what you think.

command/pr.go Outdated
if err != nil {
return err
switch err.(type) {
case *api.NotFoundError:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: a more Go 1.13-way of matching a specific error type, one that plays well with errors.Wrap https://golang.org/pkg/errors/#As

var notFoundErr *api.NotFoundError
if errors.As(err, &notFoundErr) {
  return fmt.Errorf(...)
}
return err

}

type NotFoundError struct {
message string
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: you can simplify this implementation via embedding

type NotFoundError struct {
  error
}

// usage:
return NotFoundError{fmt.Errorf(...)}

Then you don't need to define the Error() function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

😮 Nice!

@probablycorey probablycorey merged commit bac9d35 into master Dec 12, 2019
@mislav mislav deleted the better-better-better branch January 10, 2020 12:49
mislav pushed a commit that referenced this pull request Sep 28, 2021
ghcs ssh: remove terminal, bash_profile setup
Stonre pushed a commit to Stonre/strands-fork that referenced this pull request Jul 21, 2025
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.

Better error messages when you don't supply an arg

2 participants