From 305410cdeef7ca2d75647414236489e40169d6ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 10:26:57 +0100 Subject: [PATCH 1/7] Fix usage synopsis for `pr view` Indicate that the argument is optional --- command/pr.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/command/pr.go b/command/pr.go index 5204c1b3d..76eae4a3e 100644 --- a/command/pr.go +++ b/command/pr.go @@ -67,9 +67,13 @@ var prStatusCmd = &cobra.Command{ RunE: prStatus, } var prViewCmd = &cobra.Command{ - Use: "view { | | }", + Use: "view [{ | | }]", Short: "View a pull request in the browser", - RunE: prView, + Long: `View a pull request specified by the argument in the browser. + +Without an argument, the pull request that belongs to the current +branch is opened.`, + RunE: prView, } func prStatus(cmd *cobra.Command, args []string) error { From f5ad43458ccb15bd73b6f19da995271ebf19b8a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 10:28:29 +0100 Subject: [PATCH 2/7] Avoid using `<...>` in docs When converted to HTML docs, these get interpreted as HTML tags. In theory, we could encapsulate these bits in backticks, but the docs are already in raw Go string literals, and we can't easily escape backticks in that context. Instead, just avoid using `<>` for now. --- command/issue.go | 2 +- command/pr.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/command/issue.go b/command/issue.go index 139ab3337..f48549522 100644 --- a/command/issue.go +++ b/command/issue.go @@ -47,7 +47,7 @@ var issueCmd = &cobra.Command{ An issue can be supplied as argument in any of the following formats: - by number, e.g. "123"; or -- by URL, e.g. "https://github.com///issues/123".`, +- by URL, e.g. "https://github.com/OWNER/REPO/issues/123".`, } var issueCreateCmd = &cobra.Command{ Use: "create", diff --git a/command/pr.go b/command/pr.go index 76eae4a3e..e210d027c 100644 --- a/command/pr.go +++ b/command/pr.go @@ -42,8 +42,8 @@ var prCmd = &cobra.Command{ A pull request can be supplied as argument in any of the following formats: - by number, e.g. "123"; -- by URL, e.g. "https://github.com///pull/123"; or -- by the name of its head branch, e.g. "patch-1" or ":patch-1".`, +- by URL, e.g. "https://github.com/OWNER/REPO/pull/123"; or +- by the name of its head branch, e.g. "patch-1" or "OWNER:patch-1".`, } var prCheckoutCmd = &cobra.Command{ Use: "checkout { | | }", From 02f5a689374c9b3eb65f734ad57008a58148e7e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 10:45:28 +0100 Subject: [PATCH 3/7] Move main package to under `cmd/` It's a Go convention that main packages (one per each binary produced) are scoped under `cmd/`. https://github.com/github/go-lang/blob/master/docs/style-guide.md#directory-structure-and-filenames-layout --- .goreleaser.yml | 1 + Makefile | 2 +- main.go => cmd/gh/main.go | 0 3 files changed, 2 insertions(+), 1 deletion(-) rename main.go => cmd/gh/main.go (100%) diff --git a/.goreleaser.yml b/.goreleaser.yml index b3ee2336c..c8ffa8997 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -8,6 +8,7 @@ before: - go mod tidy builds: - binary: bin/gh + main: ./cmd/gh ldflags: - -s -w -X github.com/github/gh-cli/command.Version={{.Version}} -X github.com/github/gh-cli/command.BuildDate={{time "2006-01-02"}} - -X github.com/github/gh-cli/context.oauthClientID={{.Env.GH_OAUTH_CLIENT_ID}} diff --git a/Makefile b/Makefile index d17f749bf..d3cb03449 100644 --- a/Makefile +++ b/Makefile @@ -10,7 +10,7 @@ ifdef GH_OAUTH_CLIENT_SECRET endif bin/gh: $(BUILD_FILES) - @go build -ldflags "$(LDFLAGS)" -o "$@" + @go build -ldflags "$(LDFLAGS)" -o "$@" ./cmd/gh test: go test ./... diff --git a/main.go b/cmd/gh/main.go similarity index 100% rename from main.go rename to cmd/gh/main.go From 6282a3c24ede817b8ddd86f29a6176f1dafbdf91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 11:00:23 +0100 Subject: [PATCH 4/7] Improve readability of error output Ensure a blank line between error and usage output --- cmd/gh/main.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 726b50784..822e86ec6 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -28,6 +28,9 @@ func main() { fmt.Fprintln(os.Stderr, err) _, isFlagError := err.(command.FlagError) if isFlagError || strings.HasPrefix(err.Error(), "unknown command ") { + if !strings.HasSuffix(err.Error(), "\n") { + fmt.Fprintln(os.Stderr) + } fmt.Fprintln(os.Stderr, cmd.UsageString()) } os.Exit(1) From eb6541d8d676e0b25b80fd62e4585324b5764c1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 11:03:06 +0100 Subject: [PATCH 5/7] Fix CI build --- .github/workflows/go.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 2dab75d5b..40ab1acb4 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -23,4 +23,4 @@ jobs: - name: Build run: | go test ./... - go build -v . + go build -v ./cmd/gh From 537b0a842980b8ce3940b267d2c97c4d9441171f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 12:48:10 +0100 Subject: [PATCH 6/7] Friendlier output for network connectivity errors For `net.DNSError`, the full error message can be scary. Instead, print "error connecting to HOST" and hint that the user should check their internet connection or githubstatus.com. When $DEBUG is set, the full DNS error is printed like before. Fixes #206 --- cmd/gh/main.go | 37 ++++++++++++++++----- cmd/gh/main_test.go | 78 +++++++++++++++++++++++++++++++++++++++++++++ command/root.go | 12 +++++-- 3 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 cmd/gh/main_test.go diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 822e86ec6..15925a6a9 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -1,7 +1,10 @@ package main import ( + "errors" "fmt" + "io" + "net" "os" "path" "strings" @@ -12,6 +15,7 @@ import ( "github.com/github/gh-cli/utils" "github.com/mattn/go-isatty" "github.com/mgutz/ansi" + "github.com/spf13/cobra" ) var updaterEnabled = "" @@ -24,15 +28,10 @@ func main() { updateMessageChan <- rel }() + hasDebug := os.Getenv("DEBUG") != "" + if cmd, err := command.RootCmd.ExecuteC(); err != nil { - fmt.Fprintln(os.Stderr, err) - _, isFlagError := err.(command.FlagError) - if isFlagError || strings.HasPrefix(err.Error(), "unknown command ") { - if !strings.HasSuffix(err.Error(), "\n") { - fmt.Fprintln(os.Stderr) - } - fmt.Fprintln(os.Stderr, cmd.UsageString()) - } + printError(os.Stderr, err, cmd, hasDebug) os.Exit(1) } @@ -49,6 +48,28 @@ func main() { } } +func printError(out io.Writer, err error, cmd *cobra.Command, debug bool) { + var dnsError *net.DNSError + if errors.As(err, &dnsError) { + fmt.Fprintf(out, "error connecting to %s\n", dnsError.Name) + if debug { + fmt.Fprintln(out, dnsError) + } + fmt.Fprintln(out, "check your internet connection or githubstatus.com") + return + } + + fmt.Fprintln(out, err) + + var flagError *command.FlagError + if errors.As(err, &flagError) || strings.HasPrefix(err.Error(), "unknown command ") { + if !strings.HasSuffix(err.Error(), "\n") { + fmt.Fprintln(out) + } + fmt.Fprintln(out, cmd.UsageString()) + } +} + func shouldCheckForUpdate() bool { errFd := os.Stderr.Fd() return updaterEnabled != "" && (isatty.IsTerminal(errFd) || isatty.IsCygwinTerminal(errFd)) diff --git a/cmd/gh/main_test.go b/cmd/gh/main_test.go new file mode 100644 index 000000000..3a62f6470 --- /dev/null +++ b/cmd/gh/main_test.go @@ -0,0 +1,78 @@ +package main + +import ( + "bytes" + "errors" + "fmt" + "net" + "testing" + + "github.com/github/gh-cli/command" + "github.com/spf13/cobra" +) + +func Test_printError(t *testing.T) { + cmd := &cobra.Command{} + + type args struct { + err error + cmd *cobra.Command + debug bool + } + tests := []struct { + name string + args args + wantOut string + }{ + { + name: "generic error", + args: args{ + err: errors.New("the app exploded"), + cmd: nil, + debug: false, + }, + wantOut: "the app exploded\n", + }, + { + name: "DNS error", + args: args{ + err: fmt.Errorf("DNS oopsie: %w", &net.DNSError{ + Name: "api.github.com", + }), + cmd: nil, + debug: false, + }, + wantOut: `error connecting to api.github.com +check your internet connection or githubstatus.com +`, + }, + { + name: "Cobra flag error", + args: args{ + err: &command.FlagError{Err: errors.New("unknown flag --foo")}, + cmd: cmd, + debug: false, + }, + wantOut: "unknown flag --foo\n\nUsage:\n\n", + }, + { + name: "unknown Cobra command error", + args: args{ + err: errors.New("unknown command foo"), + cmd: cmd, + debug: false, + }, + wantOut: "unknown command foo\n\nUsage:\n\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := &bytes.Buffer{} + printError(out, tt.args.err, tt.args.cmd, tt.args.debug) + if gotOut := out.String(); gotOut != tt.wantOut { + t.Errorf("printError() = %q, want %q", gotOut, tt.wantOut) + } + }) + } +} diff --git a/command/root.go b/command/root.go index 5703f30c2..4141562a7 100644 --- a/command/root.go +++ b/command/root.go @@ -35,13 +35,21 @@ func init() { // RootCmd.PersistentFlags().BoolP("verbose", "V", false, "enable verbose output") RootCmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error { - return FlagError{err} + return &FlagError{Err: err} }) } // FlagError is the kind of error raised in flag processing type FlagError struct { - error + Err error +} + +func (fe FlagError) Error() string { + return fe.Err.Error() +} + +func (fe FlagError) Unwrap() error { + return fe.Err } // RootCmd is the entry point of command-line execution From f58dd040745ee68d54e21ddcba01ccaf0f358a2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 13:06:21 +0100 Subject: [PATCH 7/7] Avoid saying "number as argument" for `issue/pr view` Since issue URLs, PR URLs, and PR branch names are all accepted as arguments, avoid explicitly requesting "number" as argument. --- command/issue.go | 2 +- command/pr.go | 4 ---- command/pr_test.go | 2 +- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/command/issue.go b/command/issue.go index 139ab3337..ab966fa97 100644 --- a/command/issue.go +++ b/command/issue.go @@ -68,7 +68,7 @@ var issueViewCmd = &cobra.Command{ Use: "view { | | }", Args: func(cmd *cobra.Command, args []string) error { if len(args) < 1 { - return errors.New("requires an issue number as an argument") + return FlagError{errors.New("issue required as argument")} } return nil }, diff --git a/command/pr.go b/command/pr.go index 5204c1b3d..7b31faf51 100644 --- a/command/pr.go +++ b/command/pr.go @@ -285,10 +285,6 @@ func prView(cmd *cobra.Command, args []string) error { } else { pr, err = api.PullRequestForBranch(apiClient, baseRepo, branchWithOwner) if err != nil { - var notFoundErr *api.NotFoundError - if errors.As(err, ¬FoundErr) { - return fmt.Errorf("%s. To open a specific pull request use the pull request's number as an argument", err) - } return err } diff --git a/command/pr_test.go b/command/pr_test.go index 5a324cab1..9c17c3d4f 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -359,7 +359,7 @@ func TestPRView_noResultsForBranch(t *testing.T) { defer restoreCmd() _, err := RunCommand(prViewCmd, "pr view") - if err == nil || err.Error() != `no open pull requests found for branch "blueberries". To open a specific pull request use the pull request's number as an argument` { + if err == nil || err.Error() != `no open pull requests found for branch "blueberries"` { t.Errorf("error running command `pr view`: %v", err) }