From 16ad382f4dfbe27be609820531ad0202f6160e33 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Tue, 5 May 2020 11:35:19 -0700 Subject: [PATCH 01/23] Add simple merge test --- command/pr_test.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/command/pr_test.go b/command/pr_test.go index 0aff5e14a20..86d2829d291 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -3,6 +3,7 @@ package command import ( "bytes" "encoding/json" + "io" "io/ioutil" "os" "os/exec" @@ -938,5 +939,39 @@ func TestPRReopen_alreadyMerged(t *testing.T) { if !r.MatchString(err.Error()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } +} + +type stubResponse struct { + ResponseCode int + ResponseBody io.Reader +} + +func initWithStubs(stubs ...stubResponse) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + for _, s := range stubs { + http.StubResponse(s.ResponseCode, s.ResponseBody) + } +} + +func TestPrMerge(t *testing.T) { + initWithStubs( + stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { + "pullRequest": { "number": 1, "closed": false, "state": "OPEN"} + } } }`)}, + stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, + ) + + output, err := RunCommand("pr merge 1") + if err != nil { + t.Fatalf("error running command `pr merge`: %v", err) + } + r := regexp.MustCompile(`Merged pull request #1`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } } From 0aca0eff1fe79820813df1b768a5d1e46140c66b Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Tue, 5 May 2020 11:35:27 -0700 Subject: [PATCH 02/23] Add merge code --- command/pr.go | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/command/pr.go b/command/pr.go index 126d3db010d..313bb1f4dcf 100644 --- a/command/pr.go +++ b/command/pr.go @@ -24,6 +24,7 @@ func init() { prCmd.AddCommand(prStatusCmd) prCmd.AddCommand(prCloseCmd) prCmd.AddCommand(prReopenCmd) + prCmd.AddCommand(prMergeCmd) prCmd.AddCommand(prListCmd) prListCmd.Flags().IntP("limit", "L", 30, "Maximum number of items to fetch") @@ -80,6 +81,13 @@ var prReopenCmd = &cobra.Command{ RunE: prReopen, } +var prMergeCmd = &cobra.Command{ + Use: "merge ", + Short: "Merge a pull request", + Args: cobra.ExactArgs(1), + RunE: prMerge, +} + func prStatus(cmd *cobra.Command, args []string) error { ctx := contextForCommand(cmd) apiClient, err := apiClientForContext(ctx) @@ -420,6 +428,38 @@ func prReopen(cmd *cobra.Command, args []string) error { return nil } +func prMerge(cmd *cobra.Command, args []string) error { + ctx := contextForCommand(cmd) + apiClient, err := apiClientForContext(ctx) + if err != nil { + return err + } + + baseRepo, err := determineBaseRepo(cmd, ctx) + if err != nil { + return err + } + + pr, err := prFromArg(apiClient, baseRepo, args[0]) + if err != nil { + return err + } + + if pr.State == "MERGED" { + err := fmt.Errorf("%s Pull request #%d was already merged", utils.Red("!"), pr.Number) + return err + } + + err = api.PullRequestClose(apiClient, baseRepo, pr) + if err != nil { + return fmt.Errorf("API call failed: %w", err) + } + + fmt.Fprintf(colorableErr(cmd), "%s Merged pull request #%d\n", utils.Green("✔"), pr.Number) + + return nil +} + func printPrPreview(out io.Writer, pr *api.PullRequest) error { // Header (Title and State) fmt.Fprintln(out, utils.Bold(pr.Title)) From c0831d4c4f617e019027b12d74b6dd074276b09c Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Tue, 5 May 2020 11:56:45 -0700 Subject: [PATCH 03/23] Add merge api call --- api/queries_pr.go | 19 +++++++++++++++++++ command/pr.go | 2 +- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index ccbe1f390a7..6da60ce3172 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -799,6 +799,25 @@ func PullRequestReopen(client *Client, repo ghrepo.Interface, pr *PullRequest) e return err } +func PullRequestMerge(client *Client, repo ghrepo.Interface, pr *PullRequest) error { + var mutation struct { + ReopenPullRequest struct { + PullRequest struct { + ID githubv4.ID + } + } `graphql:"mergePullRequest(input: $input)"` + } + + input := githubv4.MergePullRequestInput{ + PullRequestID: pr.ID, + } + + v4 := githubv4.NewClient(client.http) + err := v4.Mutate(context.Background(), &mutation, input, nil) + + return err +} + func min(a, b int) int { if a < b { return a diff --git a/command/pr.go b/command/pr.go index 313bb1f4dcf..9e73e470948 100644 --- a/command/pr.go +++ b/command/pr.go @@ -450,7 +450,7 @@ func prMerge(cmd *cobra.Command, args []string) error { return err } - err = api.PullRequestClose(apiClient, baseRepo, pr) + err = api.PullRequestMerge(apiClient, baseRepo, pr) if err != nil { return fmt.Errorf("API call failed: %w", err) } From 8681e7a7b62381d6c097572ef6810fa1fecbf939 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Tue, 5 May 2020 15:09:02 -0700 Subject: [PATCH 04/23] Make squash and rebase work --- api/queries_pr.go | 13 +++++++++++++ command/pr.go | 31 +++++++++++++++++++++++++++---- command/pr_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 4 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 6da60ce3172..ffd31c75bf7 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -800,6 +800,18 @@ func PullRequestReopen(client *Client, repo ghrepo.Interface, pr *PullRequest) e } func PullRequestMerge(client *Client, repo ghrepo.Interface, pr *PullRequest) error { + return merge(client, repo, pr, githubv4.PullRequestMergeMethodMerge) +} + +func PullRequestRebase(client *Client, repo ghrepo.Interface, pr *PullRequest) error { + return merge(client, repo, pr, githubv4.PullRequestMergeMethodRebase) +} + +func PullRequestSquash(client *Client, repo ghrepo.Interface, pr *PullRequest) error { + return merge(client, repo, pr, githubv4.PullRequestMergeMethodSquash) +} + +func merge(client *Client, repo ghrepo.Interface, pr *PullRequest, mergeMethod githubv4.PullRequestMergeMethod) error { var mutation struct { ReopenPullRequest struct { PullRequest struct { @@ -810,6 +822,7 @@ func PullRequestMerge(client *Client, repo ghrepo.Interface, pr *PullRequest) er input := githubv4.MergePullRequestInput{ PullRequestID: pr.ID, + MergeMethod: &mergeMethod, } v4 := githubv4.NewClient(client.http) diff --git a/command/pr.go b/command/pr.go index 9e73e470948..4ff1a2ad36d 100644 --- a/command/pr.go +++ b/command/pr.go @@ -25,6 +25,9 @@ func init() { prCmd.AddCommand(prCloseCmd) prCmd.AddCommand(prReopenCmd) prCmd.AddCommand(prMergeCmd) + prMergeCmd.Flags().BoolP("merge", "m", true, "merge the commits with the base branch") + prMergeCmd.Flags().BoolP("rebase", "r", false, "Rebase the commits onto the base branch") + prMergeCmd.Flags().BoolP("squash", "s", false, "Squash the commits into one commit and merge it into the base branch") prCmd.AddCommand(prListCmd) prListCmd.Flags().IntP("limit", "L", 30, "Maximum number of items to fetch") @@ -58,7 +61,7 @@ var prStatusCmd = &cobra.Command{ RunE: prStatus, } var prViewCmd = &cobra.Command{ - Use: "view [{ | | }]", + Use: "view <{ | | }>", Short: "View a pull request", Long: `Display the title, body, and other information about a pull request. @@ -82,7 +85,7 @@ var prReopenCmd = &cobra.Command{ } var prMergeCmd = &cobra.Command{ - Use: "merge ", + Use: "merge [--rebase | --merge | --squash]", Short: "Merge a pull request", Args: cobra.ExactArgs(1), RunE: prMerge, @@ -450,12 +453,32 @@ func prMerge(cmd *cobra.Command, args []string) error { return err } - err = api.PullRequestMerge(apiClient, baseRepo, pr) + rebase, err := cmd.Flags().GetBool("rebase") + if err != nil { + return err + } + squash, err := cmd.Flags().GetBool("squash") + if err != nil { + return err + } + + var output string + if rebase { + output = fmt.Sprintf("%s Rebased pull request #%d\n", utils.Green("✔"), pr.Number) + err = api.PullRequestRebase(apiClient, baseRepo, pr) + } else if squash { + output = fmt.Sprintf("%s Squashed and merged pull request #%d\n", utils.Green("✔"), pr.Number) + err = api.PullRequestSquash(apiClient, baseRepo, pr) + } else { + output = fmt.Sprintf("%s Merged pull request #%d\n", utils.Green("✔"), pr.Number) + err = api.PullRequestMerge(apiClient, baseRepo, pr) + } + if err != nil { return fmt.Errorf("API call failed: %w", err) } - fmt.Fprintf(colorableErr(cmd), "%s Merged pull request #%d\n", utils.Green("✔"), pr.Number) + fmt.Fprintf(colorableErr(cmd), output) return nil } diff --git a/command/pr_test.go b/command/pr_test.go index 86d2829d291..c4d4cfb074e 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -975,3 +975,43 @@ func TestPrMerge(t *testing.T) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } } + +func TestPrMerge_rebase(t *testing.T) { + initWithStubs( + stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { + "pullRequest": { "number": 2, "closed": false, "state": "OPEN"} + } } }`)}, + stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, + ) + + output, err := RunCommand("pr merge 2 --rebase") + if err != nil { + t.Fatalf("error running command `pr merge`: %v", err) + } + + r := regexp.MustCompile(`Rebased pull request #2`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} + +func TestPrMerge_squash(t *testing.T) { + initWithStubs( + stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { + "pullRequest": { "number": 3, "closed": false, "state": "OPEN"} + } } }`)}, + stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, + ) + + output, err := RunCommand("pr merge 3 --squash") + if err != nil { + t.Fatalf("error running command `pr merge`: %v", err) + } + + r := regexp.MustCompile(`Squashed and merged pull request #3`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} From 062c8353eb51efd0e01d648a0fc1bb8673028e23 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Tue, 5 May 2020 15:11:06 -0700 Subject: [PATCH 05/23] Add 'already merged' test --- command/pr_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/command/pr_test.go b/command/pr_test.go index c4d4cfb074e..9d75a482de8 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -1015,3 +1015,23 @@ func TestPrMerge_squash(t *testing.T) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } } + +func TestPrMerge_alreadyMerged(t *testing.T) { + initWithStubs( + stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { + "pullRequest": { "number": 4, "closed": true, "state": "MERGED"} + } } }`)}, + stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, + ) + + output, err := RunCommand("pr merge 4") + if err == nil { + t.Fatalf("expected an error running command `pr merge`: %v", err) + } + + r := regexp.MustCompile(`Pull request #4 was already merged`) + + if !r.MatchString(err.Error()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} From 8c85e14bac8f0bd47b8f0c8a16f9e23c314e8d98 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Wed, 6 May 2020 11:19:00 -0700 Subject: [PATCH 06/23] Make it work without a PR number --- api/queries_pr.go | 2 ++ command/pr.go | 25 ++++++++++++++++++++----- command/pr_test.go | 20 ++++++++++++++++++++ 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index ffd31c75bf7..0c42178f72d 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -446,6 +446,7 @@ func PullRequestForBranch(client *Client, repo ghrepo.Interface, baseBranch, hea type response struct { Repository struct { PullRequests struct { + ID githubv4.ID Nodes []PullRequest } } @@ -456,6 +457,7 @@ func PullRequestForBranch(client *Client, repo ghrepo.Interface, baseBranch, hea repository(owner: $owner, name: $repo) { pullRequests(headRefName: $headRefName, states: OPEN, first: 30) { nodes { + id number title state diff --git a/command/pr.go b/command/pr.go index 4ff1a2ad36d..e0b80871f15 100644 --- a/command/pr.go +++ b/command/pr.go @@ -85,9 +85,9 @@ var prReopenCmd = &cobra.Command{ } var prMergeCmd = &cobra.Command{ - Use: "merge [--rebase | --merge | --squash]", + Use: "merge [number | url] [--rebase | --merge | --squash]", Short: "Merge a pull request", - Args: cobra.ExactArgs(1), + Args: cobra.MaximumNArgs(1), RunE: prMerge, } @@ -110,6 +110,7 @@ func prStatus(cmd *cobra.Command, args []string) error { repoOverride, _ := cmd.Flags().GetString("repo") currentPRNumber, currentPRHeadRef, err := prSelectorForCurrentBranch(ctx, baseRepo) + if err != nil && repoOverride == "" && err.Error() != "git: not on any branch" { return fmt.Errorf("could not query for pull request for current branch: %w", err) } @@ -443,9 +444,23 @@ func prMerge(cmd *cobra.Command, args []string) error { return err } - pr, err := prFromArg(apiClient, baseRepo, args[0]) - if err != nil { - return err + var pr *api.PullRequest + if len(args) > 0 { + pr, err = prFromArg(apiClient, baseRepo, args[0]) + if err != nil { + return err + } + } else { + _, branchWithOwner, err := prSelectorForCurrentBranch(ctx, baseRepo) + if err != nil { + return err + } + + pr, err = api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) + fmt.Printf("🌭 %+v\n", pr) + if err != nil { + return err + } } if pr.State == "MERGED" { diff --git a/command/pr_test.go b/command/pr_test.go index 9d75a482de8..6b5be7eb6a7 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -976,6 +976,26 @@ func TestPrMerge(t *testing.T) { } } +func TestPrMerge_noPrNumberGiven(t *testing.T) { + initWithStubs( + stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { + "pullRequest": { "number": 100, "closed": false, "state": "OPEN"} + } } }`)}, + stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, + ) + + output, err := RunCommand("pr merge") + if err != nil { + t.Fatalf("error running command `pr merge`: %v", err) + } + + r := regexp.MustCompile(`Merged pull request #100`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} + func TestPrMerge_rebase(t *testing.T) { initWithStubs( stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { From 5a04679535b2ae2c4b2bb6fe883bcbdd8a57c26c Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Wed, 6 May 2020 11:21:05 -0700 Subject: [PATCH 07/23] Remove debug output --- command/pr.go | 1 - 1 file changed, 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index e0b80871f15..ad76a2e1dd9 100644 --- a/command/pr.go +++ b/command/pr.go @@ -457,7 +457,6 @@ func prMerge(cmd *cobra.Command, args []string) error { } pr, err = api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) - fmt.Printf("🌭 %+v\n", pr) if err != nil { return err } From 1de57db74de9045cf5b8089d3edd02fa48cbc3ff Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Wed, 6 May 2020 11:27:41 -0700 Subject: [PATCH 08/23] Fix lint error --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index ad76a2e1dd9..7b36089b55e 100644 --- a/command/pr.go +++ b/command/pr.go @@ -492,7 +492,7 @@ func prMerge(cmd *cobra.Command, args []string) error { return fmt.Errorf("API call failed: %w", err) } - fmt.Fprintf(colorableErr(cmd), output) + fmt.Fprint(colorableErr(cmd), output) return nil } From 3d21a33bac0056d05fbaf604a8aa94cc17c72f61 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Wed, 6 May 2020 11:47:43 -0700 Subject: [PATCH 09/23] update test --- command/pr_test.go | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/command/pr_test.go b/command/pr_test.go index 6b5be7eb6a7..4bbba2eae91 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -946,8 +946,8 @@ type stubResponse struct { ResponseBody io.Reader } -func initWithStubs(stubs ...stubResponse) { - initBlankContext("", "OWNER/REPO", "master") +func initWithStubs(branch string, stubs ...stubResponse) { + initBlankContext("", "OWNER/REPO", branch) http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -957,7 +957,7 @@ func initWithStubs(stubs ...stubResponse) { } func TestPrMerge(t *testing.T) { - initWithStubs( + initWithStubs("master", stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { "pullRequest": { "number": 1, "closed": false, "state": "OPEN"} } } }`)}, @@ -977,10 +977,16 @@ func TestPrMerge(t *testing.T) { } func TestPrMerge_noPrNumberGiven(t *testing.T) { - initWithStubs( - stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { - "pullRequest": { "number": 100, "closed": false, "state": "OPEN"} - } } }`)}, + cs, cmdTeardown := test.InitCmdStubber() + defer cmdTeardown() + + cs.Stub("branch.blueberries.remote origin\nbranch.blueberries.merge refs/heads/blueberries") // git config --get-regexp ^branch\.master\.(remote|merge) + + jsonFile, _ := os.Open("../test/fixtures/prViewPreviewWithMetadataByBranch.json") + defer jsonFile.Close() + + initWithStubs("blueberries", + stubResponse{200, jsonFile}, stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, ) @@ -989,7 +995,7 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) { t.Fatalf("error running command `pr merge`: %v", err) } - r := regexp.MustCompile(`Merged pull request #100`) + r := regexp.MustCompile(`Merged pull request #10`) if !r.MatchString(output.Stderr()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) @@ -997,7 +1003,7 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) { } func TestPrMerge_rebase(t *testing.T) { - initWithStubs( + initWithStubs("master", stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { "pullRequest": { "number": 2, "closed": false, "state": "OPEN"} } } }`)}, @@ -1017,7 +1023,7 @@ func TestPrMerge_rebase(t *testing.T) { } func TestPrMerge_squash(t *testing.T) { - initWithStubs( + initWithStubs("master", stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { "pullRequest": { "number": 3, "closed": false, "state": "OPEN"} } } }`)}, @@ -1037,7 +1043,7 @@ func TestPrMerge_squash(t *testing.T) { } func TestPrMerge_alreadyMerged(t *testing.T) { - initWithStubs( + initWithStubs("master", stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { "pullRequest": { "number": 4, "closed": true, "state": "MERGED"} } } }`)}, From 5810acf6ac72df3d29a1b2a50fe6461e03c9192f Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 7 May 2020 10:29:56 -0700 Subject: [PATCH 10/23] Use correct struct --- api/queries_pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 0c42178f72d..bcf589534b7 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -815,7 +815,7 @@ func PullRequestSquash(client *Client, repo ghrepo.Interface, pr *PullRequest) e func merge(client *Client, repo ghrepo.Interface, pr *PullRequest, mergeMethod githubv4.PullRequestMergeMethod) error { var mutation struct { - ReopenPullRequest struct { + MergePullRequest struct { PullRequest struct { ID githubv4.ID } From bcf41fd5e7da411b068d6323c12ca5c571bf28e5 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 7 May 2020 10:30:14 -0700 Subject: [PATCH 11/23] Fix sentence case --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index 7b36089b55e..1d3cf704e42 100644 --- a/command/pr.go +++ b/command/pr.go @@ -25,7 +25,7 @@ func init() { prCmd.AddCommand(prCloseCmd) prCmd.AddCommand(prReopenCmd) prCmd.AddCommand(prMergeCmd) - prMergeCmd.Flags().BoolP("merge", "m", true, "merge the commits with the base branch") + prMergeCmd.Flags().BoolP("merge", "m", true, "Merge the commits with the base branch") prMergeCmd.Flags().BoolP("rebase", "r", false, "Rebase the commits onto the base branch") prMergeCmd.Flags().BoolP("squash", "s", false, "Squash the commits into one commit and merge it into the base branch") From 2041f0ab1b0b8c643ff8a446ee0238cf1e845155 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 7 May 2020 10:30:23 -0700 Subject: [PATCH 12/23] Fix usage syntax --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index 1d3cf704e42..0b134a30996 100644 --- a/command/pr.go +++ b/command/pr.go @@ -61,7 +61,7 @@ var prStatusCmd = &cobra.Command{ RunE: prStatus, } var prViewCmd = &cobra.Command{ - Use: "view <{ | | }>", + Use: "view [{ | | }]", Short: "View a pull request", Long: `Display the title, body, and other information about a pull request. From 1ea38af79c41d1ef2041676202f3efbee7654759 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 7 May 2020 10:30:31 -0700 Subject: [PATCH 13/23] Fix merge usage syntax --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index 0b134a30996..c7f74d256a3 100644 --- a/command/pr.go +++ b/command/pr.go @@ -85,7 +85,7 @@ var prReopenCmd = &cobra.Command{ } var prMergeCmd = &cobra.Command{ - Use: "merge [number | url] [--rebase | --merge | --squash]", + Use: "merge [{ | }] []", Short: "Merge a pull request", Args: cobra.MaximumNArgs(1), RunE: prMerge, From 4b2f14d9390d563e69680de68dac51e28e7d2bed Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 7 May 2020 10:30:51 -0700 Subject: [PATCH 14/23] Use PR if given --- command/pr.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/command/pr.go b/command/pr.go index c7f74d256a3..9691b9ba7b3 100644 --- a/command/pr.go +++ b/command/pr.go @@ -451,12 +451,16 @@ func prMerge(cmd *cobra.Command, args []string) error { return err } } else { - _, branchWithOwner, err := prSelectorForCurrentBranch(ctx, baseRepo) + prNumber, branchWithOwner, err := prSelectorForCurrentBranch(ctx, baseRepo) if err != nil { return err } - pr, err = api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) + if prNumber != 0 { + pr, err = api.PullRequestByNumber(apiClient, baseRepo, prNumber) + } else { + pr, err = api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) + } if err != nil { return err } From 5b78d47306cf12ba6d94e50ac64f08bf75ad2b7d Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 7 May 2020 10:30:59 -0700 Subject: [PATCH 15/23] Use stdout --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index 9691b9ba7b3..8678fcb901c 100644 --- a/command/pr.go +++ b/command/pr.go @@ -496,7 +496,7 @@ func prMerge(cmd *cobra.Command, args []string) error { return fmt.Errorf("API call failed: %w", err) } - fmt.Fprint(colorableErr(cmd), output) + fmt.Fprint(colorableOut(cmd), output) return nil } From 58663dccfdf857dd66dddcad9e3449b337dba1ab Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 7 May 2020 10:31:05 -0700 Subject: [PATCH 16/23] Better description --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index 8678fcb901c..e5cd8b5141b 100644 --- a/command/pr.go +++ b/command/pr.go @@ -482,7 +482,7 @@ func prMerge(cmd *cobra.Command, args []string) error { var output string if rebase { - output = fmt.Sprintf("%s Rebased pull request #%d\n", utils.Green("✔"), pr.Number) + output = fmt.Sprintf("%s Rebased and merged pull request #%d\n", utils.Green("✔"), pr.Number) err = api.PullRequestRebase(apiClient, baseRepo, pr) } else if squash { output = fmt.Sprintf("%s Squashed and merged pull request #%d\n", utils.Green("✔"), pr.Number) From ea6b3dca8ca403b2867d077e534697a3306dae01 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 7 May 2020 10:52:30 -0700 Subject: [PATCH 17/23] Update pr_test.go --- command/pr_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/command/pr_test.go b/command/pr_test.go index 4bbba2eae91..61971d5387a 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -971,7 +971,7 @@ func TestPrMerge(t *testing.T) { r := regexp.MustCompile(`Merged pull request #1`) - if !r.MatchString(output.Stderr()) { + if !r.MatchString(output.String()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } } @@ -997,7 +997,7 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) { r := regexp.MustCompile(`Merged pull request #10`) - if !r.MatchString(output.Stderr()) { + if !r.MatchString(output.String()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } } @@ -1015,9 +1015,9 @@ func TestPrMerge_rebase(t *testing.T) { t.Fatalf("error running command `pr merge`: %v", err) } - r := regexp.MustCompile(`Rebased pull request #2`) + r := regexp.MustCompile(`Rebased and merged pull request #2`) - if !r.MatchString(output.Stderr()) { + if !r.MatchString(output.String()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } } @@ -1037,7 +1037,7 @@ func TestPrMerge_squash(t *testing.T) { r := regexp.MustCompile(`Squashed and merged pull request #3`) - if !r.MatchString(output.Stderr()) { + if !r.MatchString(output.String()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } } From b9c4a7668765c0f3c4c33b9d66768683fac038d5 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 8 May 2020 11:20:00 -0700 Subject: [PATCH 18/23] Update usage docs --- command/pr.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/command/pr.go b/command/pr.go index e5cd8b5141b..d245349db55 100644 --- a/command/pr.go +++ b/command/pr.go @@ -61,7 +61,7 @@ var prStatusCmd = &cobra.Command{ RunE: prStatus, } var prViewCmd = &cobra.Command{ - Use: "view [{ | | }]", + Use: "view [ | | ]", Short: "View a pull request", Long: `Display the title, body, and other information about a pull request. @@ -72,20 +72,20 @@ With '--web', open the pull request in a web browser instead.`, RunE: prView, } var prCloseCmd = &cobra.Command{ - Use: "close ", + Use: "close { | | }", Short: "Close a pull request", Args: cobra.ExactArgs(1), RunE: prClose, } var prReopenCmd = &cobra.Command{ - Use: "reopen ", + Use: "reopen { | | }", Short: "Reopen a pull request", Args: cobra.ExactArgs(1), RunE: prReopen, } var prMergeCmd = &cobra.Command{ - Use: "merge [{ | }] []", + Use: "merge [ | | ]", Short: "Merge a pull request", Args: cobra.MaximumNArgs(1), RunE: prMerge, From 43e15130f1db1086fa5e0fe1c368c3ecded03c44 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 8 May 2020 11:37:45 -0700 Subject: [PATCH 19/23] Use a var --- api/queries_pr.go | 28 ++++++++++++++++------------ command/pr.go | 6 +++--- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index bcf589534b7..180b7091f18 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -130,6 +130,14 @@ type PullRequestReviewStatus struct { ReviewRequired bool } +type PullRequestMergeMethod int + +const ( + PullRequestMergeMethodMerge PullRequestMergeMethod = iota + PullRequestMergeMethodRebase + PullRequestMergeMethodSquash +) + func (pr *PullRequest) ReviewStatus() PullRequestReviewStatus { var status PullRequestReviewStatus switch pr.ReviewDecision { @@ -801,19 +809,15 @@ func PullRequestReopen(client *Client, repo ghrepo.Interface, pr *PullRequest) e return err } -func PullRequestMerge(client *Client, repo ghrepo.Interface, pr *PullRequest) error { - return merge(client, repo, pr, githubv4.PullRequestMergeMethodMerge) -} - -func PullRequestRebase(client *Client, repo ghrepo.Interface, pr *PullRequest) error { - return merge(client, repo, pr, githubv4.PullRequestMergeMethodRebase) -} - -func PullRequestSquash(client *Client, repo ghrepo.Interface, pr *PullRequest) error { - return merge(client, repo, pr, githubv4.PullRequestMergeMethodSquash) -} +func PullRequestMerge(client *Client, repo ghrepo.Interface, pr *PullRequest, m PullRequestMergeMethod) error { + mergeMethod := githubv4.PullRequestMergeMethodMerge + switch m { + case PullRequestMergeMethodRebase: + mergeMethod = githubv4.PullRequestMergeMethodRebase + case PullRequestMergeMethodSquash: + mergeMethod = githubv4.PullRequestMergeMethodSquash + } -func merge(client *Client, repo ghrepo.Interface, pr *PullRequest, mergeMethod githubv4.PullRequestMergeMethod) error { var mutation struct { MergePullRequest struct { PullRequest struct { diff --git a/command/pr.go b/command/pr.go index d245349db55..e590c0e7086 100644 --- a/command/pr.go +++ b/command/pr.go @@ -483,13 +483,13 @@ func prMerge(cmd *cobra.Command, args []string) error { var output string if rebase { output = fmt.Sprintf("%s Rebased and merged pull request #%d\n", utils.Green("✔"), pr.Number) - err = api.PullRequestRebase(apiClient, baseRepo, pr) + err = api.PullRequestMerge(apiClient, baseRepo, pr, api.PullRequestMergeMethodRebase) } else if squash { output = fmt.Sprintf("%s Squashed and merged pull request #%d\n", utils.Green("✔"), pr.Number) - err = api.PullRequestSquash(apiClient, baseRepo, pr) + err = api.PullRequestMerge(apiClient, baseRepo, pr, api.PullRequestMergeMethodSquash) } else { output = fmt.Sprintf("%s Merged pull request #%d\n", utils.Green("✔"), pr.Number) - err = api.PullRequestMerge(apiClient, baseRepo, pr) + err = api.PullRequestMerge(apiClient, baseRepo, pr, api.PullRequestMergeMethodMerge) } if err != nil { From c8e9768bf5bbce3f0edb0eb95b83998a2028c471 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 11 May 2020 15:28:09 -0500 Subject: [PATCH 20/23] slopwip on interactive review --- command/pr_review.go | 125 +++++++++++++++++++++++++- command/pr_review_test.go | 166 ++++++++++++++++++++++++++++++++++- command/root.go | 14 +++ command/title_body_survey.go | 12 +-- 4 files changed, 305 insertions(+), 12 deletions(-) diff --git a/command/pr_review.go b/command/pr_review.go index 5013d2bc226..92b19b6ee3f 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -6,8 +6,12 @@ import ( "strconv" "strings" - "github.com/cli/cli/api" + "github.com/AlecAivazis/survey/v2" "github.com/spf13/cobra" + + "github.com/cli/cli/api" + "github.com/cli/cli/pkg/surveyext" + "github.com/cli/cli/utils" ) func init() { @@ -28,6 +32,8 @@ var prReviewCmd = &cobra.Command{ Examples: + gh pr review # add a review for the current branch's pull request + gh pr review 123 # add a review for pull request 123 gh pr review -a # mark the current branch's pull request as approved gh pr review -c -b "interesting" # comment on the current branch's pull request gh pr review 123 -r -b "needs more ascii art" # request changes on pull request 123 @@ -56,7 +62,9 @@ func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { state = api.ReviewComment } - if found != 1 { + if found == 0 { + return nil, nil // signal interactive mode + } else if found > 1 { return nil, errors.New("need exactly one of --approve, --request-changes, or --comment") } @@ -126,6 +134,17 @@ func prReview(cmd *cobra.Command, args []string) error { } } + if input == nil { + input, err = reviewSurvey(cmd) + if err != nil { + return err + } + if input == nil && err == nil { + // Cancelled. + return nil + } + } + err = api.AddReview(apiClient, pr, input) if err != nil { return fmt.Errorf("failed to create review: %w", err) @@ -133,3 +152,105 @@ func prReview(cmd *cobra.Command, args []string) error { return nil } + +func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { + editorCommand, err := determineEditor(cmd) + if err != nil { + return nil, err + } + + typeAnswers := struct { + ReviewType string + }{} + typeQs := []*survey.Question{ + { + Name: "reviewType", + Prompt: &survey.Select{ + Message: "What kind of review do you want to give?", + Options: []string{ + "Comment", + "Approve", + "Request changes", + "Cancel", + }, + }, + }, + } + + err = SurveyAsk(typeQs, &typeAnswers) + if err != nil { + return nil, err + } + + reviewState := api.ReviewComment + + switch typeAnswers.ReviewType { + case "Approve": + reviewState = api.ReviewApprove + case "Request Changes": + reviewState = api.ReviewRequestChanges + case "Cancel": + return nil, nil + } + + bodyAnswers := struct { + Body string + }{} + + bodyQs := []*survey.Question{ + &survey.Question{ + Name: "body", + Prompt: &surveyext.GhEditor{ + EditorCommand: editorCommand, + Editor: &survey.Editor{ + Message: "Review body", + FileName: "*.md", + }, + }, + }, + } + + err = SurveyAsk(bodyQs, &bodyAnswers) + if err != nil { + return nil, err + } + + if bodyAnswers.Body == "" && (reviewState == api.ReviewComment || reviewState == api.ReviewRequestChanges) { + return nil, errors.New("this type of review cannot be blank") + } + + if len(bodyAnswers.Body) > 0 { + out := colorableOut(cmd) + renderedBody, err := utils.RenderMarkdown(bodyAnswers.Body) + if err != nil { + return nil, err + } + + fmt.Fprintf(out, "Got:\n%s", renderedBody) + } + + confirm := false + confirmQs := []*survey.Question{ + { + Name: "confirm", + Prompt: &survey.Confirm{ + Message: "Submit?", + Default: true, + }, + }, + } + + err = SurveyAsk(confirmQs, &confirm) + if err != nil { + return nil, err + } + + if !confirm { + return nil, nil + } + + return &api.PullRequestReviewInput{ + Body: bodyAnswers.Body, + State: reviewState, + }, nil +} diff --git a/command/pr_review_test.go b/command/pr_review_test.go index 781b2cb0656..17b24a74277 100644 --- a/command/pr_review_test.go +++ b/command/pr_review_test.go @@ -4,7 +4,10 @@ import ( "bytes" "encoding/json" "io/ioutil" + "regexp" "testing" + + "github.com/cli/cli/test" ) func TestPRReview_validation(t *testing.T) { @@ -12,7 +15,6 @@ func TestPRReview_validation(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() for _, cmd := range []string{ - `pr review 123`, `pr review --approve --comment 123`, `pr review --approve --comment -b"hey" 123`, } { @@ -218,3 +220,165 @@ func TestPRReview(t *testing.T) { eq(t, reqBody.Variables.Input.Body, kase.ExpectedBody) } } + +func TestPRReview_interactive(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + as, teardown := initAskStubber() + defer teardown() + + as.Stub([]*QuestionStub{ + { + Name: "reviewType", + Value: "Approve", + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "body", + Value: "cool story", + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "confirm", + Value: true, + }, + }) + + output, err := RunCommand(`pr review`) + if err != nil { + t.Fatalf("got unexpected error running pr review: %s", err) + } + + test.ExpectLines(t, output.String(), + "Got:", + "cool.*story") // weird because markdown rendering puts a bunch of junk between works + + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + reqBody := struct { + Variables struct { + Input struct { + Event string + Body string + } + } + }{} + _ = json.Unmarshal(bodyBytes, &reqBody) + + eq(t, reqBody.Variables.Input.Event, "APPROVE") + eq(t, reqBody.Variables.Input.Body, "cool story") +} + +func TestPRReview_interactive_no_body(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + as, teardown := initAskStubber() + defer teardown() + + as.Stub([]*QuestionStub{ + { + Name: "reviewType", + Value: "Request Changes", + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "body", + Default: true, + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "confirm", + Value: true, + }, + }) + + _, err := RunCommand(`pr review`) + if err == nil { + t.Fatal("expected error") + } + eq(t, err.Error(), "this type of review cannot be blank") +} + +func TestPRReview_interactive_blank_approve(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + as, teardown := initAskStubber() + defer teardown() + + as.Stub([]*QuestionStub{ + { + Name: "reviewType", + Value: "Approve", + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "body", + Default: true, + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "confirm", + Value: true, + }, + }) + + output, err := RunCommand(`pr review`) + if err != nil { + t.Fatalf("got unexpected error running pr review: %s", err) + } + + unexpect := regexp.MustCompile("Got:") + if unexpect.MatchString(output.String()) { + t.Errorf("did not expect to see body printed in %s", output.String()) + } + + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + reqBody := struct { + Variables struct { + Input struct { + Event string + Body string + } + } + }{} + _ = json.Unmarshal(bodyBytes, &reqBody) + + eq(t, reqBody.Variables.Input.Event, "APPROVE") + eq(t, reqBody.Variables.Input.Body, "") + +} diff --git a/command/root.go b/command/root.go index 3ef268cb79a..ef88a3184f5 100644 --- a/command/root.go +++ b/command/root.go @@ -338,3 +338,17 @@ func formatRemoteURL(cmd *cobra.Command, fullRepoName string) string { return fmt.Sprintf("https://%s/%s.git", defaultHostname, fullRepoName) } + +func determineEditor(cmd *cobra.Command) (string, error) { + editorCommand := os.Getenv("GH_EDITOR") + if editorCommand == "" { + ctx := contextForCommand(cmd) + cfg, err := ctx.Config() + if err != nil { + return "", fmt.Errorf("could not read config: %w", err) + } + editorCommand, _ = cfg.Get(defaultHostname, "editor") + } + + return editorCommand, nil +} diff --git a/command/title_body_survey.go b/command/title_body_survey.go index 06ab14a096d..37c20208098 100644 --- a/command/title_body_survey.go +++ b/command/title_body_survey.go @@ -2,7 +2,6 @@ package command import ( "fmt" - "os" "github.com/AlecAivazis/survey/v2" "github.com/cli/cli/api" @@ -127,14 +126,9 @@ func selectTemplate(templatePaths []string) (string, error) { } func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClient *api.Client, repo ghrepo.Interface, providedTitle, providedBody string, defs defaults, templatePaths []string, allowReviewers, allowMetadata bool) error { - editorCommand := os.Getenv("GH_EDITOR") - if editorCommand == "" { - ctx := contextForCommand(cmd) - cfg, err := ctx.Config() - if err != nil { - return fmt.Errorf("could not read config: %w", err) - } - editorCommand, _ = cfg.Get(defaultHostname, "editor") + editorCommand, err := determineEditor(cmd) + if err != nil { + return err } issueState.Title = defs.Title From 700932131467d5a3d35c22305f36e083c2c35017 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 11 May 2020 15:29:10 -0500 Subject: [PATCH 21/23] review feedback --- command/pr_review.go | 50 ++++++++++++++++++++++++------------ command/pr_review_test.go | 39 ++++++++++++++++++++-------- command/title_body_survey.go | 2 +- pkg/surveyext/editor.go | 18 +++++++++---- 4 files changed, 76 insertions(+), 33 deletions(-) diff --git a/command/pr_review.go b/command/pr_review.go index 92b19b6ee3f..3b01d0d5b35 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -15,8 +15,7 @@ import ( ) func init() { - // TODO re-register post release - // prCmd.AddCommand(prReviewCmd) + prCmd.AddCommand(prReviewCmd) prReviewCmd.Flags().BoolP("approve", "a", false, "Approve pull request") prReviewCmd.Flags().BoolP("request-changes", "r", false, "Request changes on a pull request") @@ -62,17 +61,19 @@ func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { state = api.ReviewComment } - if found == 0 { - return nil, nil // signal interactive mode - } else if found > 1 { - return nil, errors.New("need exactly one of --approve, --request-changes, or --comment") - } - body, err := cmd.Flags().GetString("body") if err != nil { return nil, err } + if found == 0 && body == "" { + return nil, nil // signal interactive mode + } else if found == 0 && body != "" { + return nil, errors.New("--body unsupported without --approve, --request-changes, or --comment") + } else if found > 1 { + return nil, errors.New("need exactly one of --approve, --request-changes, or --comment") + } + if (flag == "request-changes" || flag == "comment") && body == "" { return nil, fmt.Errorf("body cannot be blank for %s review", flag) } @@ -116,7 +117,7 @@ func prReview(cmd *cobra.Command, args []string) error { } } - input, err := processReviewOpt(cmd) + reviewData, err := processReviewOpt(cmd) if err != nil { return fmt.Errorf("did not understand desired review action: %w", err) } @@ -132,24 +133,36 @@ func prReview(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("could not find pull request: %w", err) } + prNum = pr.Number } - if input == nil { - input, err = reviewSurvey(cmd) + out := colorableOut(cmd) + + if reviewData == nil { + reviewData, err = reviewSurvey(cmd) if err != nil { return err } - if input == nil && err == nil { - // Cancelled. + if reviewData == nil && err == nil { + fmt.Fprint(out, "Discarding.\n") return nil } } - err = api.AddReview(apiClient, pr, input) + err = api.AddReview(apiClient, pr, reviewData) if err != nil { return fmt.Errorf("failed to create review: %w", err) } + switch reviewData.State { + case api.ReviewComment: + fmt.Fprintf(out, "%s Reviewed pull request #%d\n", utils.Gray("-"), prNum) + case api.ReviewApprove: + fmt.Fprintf(out, "%s Approved pull request #%d\n", utils.Green("✓"), prNum) + case api.ReviewRequestChanges: + fmt.Fprintf(out, "%s Requested changes to pull request #%d\n", utils.Red("+"), prNum) + } + return nil } @@ -171,7 +184,6 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { "Comment", "Approve", "Request changes", - "Cancel", }, }, }, @@ -189,18 +201,22 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { reviewState = api.ReviewApprove case "Request Changes": reviewState = api.ReviewRequestChanges - case "Cancel": - return nil, nil } bodyAnswers := struct { Body string }{} + blankAllowed := false + if reviewState == api.ReviewApprove { + blankAllowed = true + } + bodyQs := []*survey.Question{ &survey.Question{ Name: "body", Prompt: &surveyext.GhEditor{ + BlankAllowed: blankAllowed, EditorCommand: editorCommand, Editor: &survey.Editor{ Message: "Review body", diff --git a/command/pr_review_test.go b/command/pr_review_test.go index 17b24a74277..231906ac080 100644 --- a/command/pr_review_test.go +++ b/command/pr_review_test.go @@ -11,7 +11,6 @@ import ( ) func TestPRReview_validation(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() for _, cmd := range []string{ @@ -20,12 +19,25 @@ func TestPRReview_validation(t *testing.T) { } { http.StubRepoResponse("OWNER", "REPO") _, err := RunCommand(cmd) + if err == nil { + t.Fatal("expected error") + } eq(t, err.Error(), "did not understand desired review action: need exactly one of --approve, --request-changes, or --comment") } } +func TestPRReview_bad_body(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + _, err := RunCommand(`pr review -b "radical"`) + if err == nil { + t.Fatal("expected error") + } + eq(t, err.Error(), "did not understand desired review action: --body unsupported without --approve, --request-changes, or --comment") +} + func TestPRReview_url_arg(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -48,11 +60,13 @@ func TestPRReview_url_arg(t *testing.T) { } } } } `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - _, err := RunCommand("pr review --approve https://github.com/OWNER/REPO/pull/123") + output, err := RunCommand("pr review --approve https://github.com/OWNER/REPO/pull/123") if err != nil { t.Fatalf("error running pr review: %s", err) } + test.ExpectLines(t, output.String(), "Approved pull request #123") + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { Variables struct { @@ -71,7 +85,6 @@ func TestPRReview_url_arg(t *testing.T) { } func TestPRReview_number_arg(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -94,11 +107,13 @@ func TestPRReview_number_arg(t *testing.T) { } } } } `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - _, err := RunCommand("pr review --approve 123") + output, err := RunCommand("pr review --approve 123") if err != nil { t.Fatalf("error running pr review: %s", err) } + test.ExpectLines(t, output.String(), "Approved pull request #123") + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { Variables struct { @@ -117,24 +132,26 @@ func TestPRReview_number_arg(t *testing.T) { } func TestPRReview_no_arg(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "feature") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, "id": "foobar123", "headRefName": "feature", "baseRefName": "master" } ] } } } }`)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - _, err := RunCommand(`pr review --comment -b "cool story"`) + output, err := RunCommand(`pr review --comment -b "cool story"`) if err != nil { t.Fatalf("error running pr review: %s", err) } + test.ExpectLines(t, output.String(), "- Reviewed pull request #123") + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { Variables struct { @@ -153,7 +170,6 @@ func TestPRReview_no_arg(t *testing.T) { } func TestPRReview_blank_comment(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -163,7 +179,6 @@ func TestPRReview_blank_comment(t *testing.T) { } func TestPRReview_blank_request_changes(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -173,7 +188,6 @@ func TestPRReview_blank_request_changes(t *testing.T) { } func TestPRReview(t *testing.T) { - t.Skip("skipping until release is done") type c struct { Cmd string ExpectedEvent string @@ -228,6 +242,7 @@ func TestPRReview_interactive(t *testing.T) { http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, "id": "foobar123", "headRefName": "feature", "baseRefName": "master" } @@ -262,6 +277,7 @@ func TestPRReview_interactive(t *testing.T) { } test.ExpectLines(t, output.String(), + "Approved pull request #123", "Got:", "cool.*story") // weird because markdown rendering puts a bunch of junk between works @@ -329,6 +345,7 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, "id": "foobar123", "headRefName": "feature", "baseRefName": "master" } @@ -367,6 +384,8 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { t.Errorf("did not expect to see body printed in %s", output.String()) } + test.ExpectLines(t, output.String(), "Approved pull request #123") + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { Variables struct { diff --git a/command/title_body_survey.go b/command/title_body_survey.go index 37c20208098..488efa34a13 100644 --- a/command/title_body_survey.go +++ b/command/title_body_survey.go @@ -176,7 +176,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie qs = append(qs, bodyQuestion) } - err := SurveyAsk(qs, issueState) + err = SurveyAsk(qs, issueState) if err != nil { return fmt.Errorf("could not prompt: %w", err) } diff --git a/pkg/surveyext/editor.go b/pkg/surveyext/editor.go index 3121ccf4d4b..9c1fca2e1f3 100644 --- a/pkg/surveyext/editor.go +++ b/pkg/surveyext/editor.go @@ -38,6 +38,7 @@ func init() { type GhEditor struct { *survey.Editor EditorCommand string + BlankAllowed bool } func (e *GhEditor) editorCommand() string { @@ -58,13 +59,14 @@ var EditorQuestionTemplate = ` {{- else }} {{- if and .Help (not .ShowHelp)}}{{color "cyan"}}[{{ .Config.HelpInput }} for help]{{color "reset"}} {{end}} {{- if and .Default (not .HideDefault)}}{{color "white"}}({{.Default}}) {{color "reset"}}{{end}} - {{- color "cyan"}}[(e) to launch {{ .EditorCommand }}, enter to skip] {{color "reset"}} + {{- color "cyan"}}[(e) to launch {{ .EditorCommand }}{{- if .BlankAllowed }}, enter to skip{{ end }}] {{color "reset"}} {{- end}}` // EXTENDED to pass editor name (to use in prompt) type EditorTemplateData struct { survey.Editor EditorCommand string + BlankAllowed bool Answer string ShowAnswer bool ShowHelp bool @@ -75,9 +77,10 @@ type EditorTemplateData struct { func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (interface{}, error) { err := e.Render( EditorQuestionTemplate, - // EXTENDED to support printing editor in prompt + // EXTENDED to support printing editor in prompt and BlankAllowed EditorTemplateData{ Editor: *e.Editor, + BlankAllowed: e.BlankAllowed, EditorCommand: filepath.Base(e.editorCommand()), Config: config, }, @@ -96,7 +99,7 @@ func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (int defer cursor.Show() for { - // EXTENDED to handle the e to edit / enter to skip behavior + // EXTENDED to handle the e to edit / enter to skip behavior + BlankAllowed r, _, err := rr.ReadRune() if err != nil { return "", err @@ -105,7 +108,11 @@ func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (int break } if r == '\r' || r == '\n' { - return "", nil + if e.BlankAllowed { + return "", nil + } else { + continue + } } if r == terminal.KeyInterrupt { return "", terminal.InterruptErr @@ -117,8 +124,9 @@ func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (int err = e.Render( EditorQuestionTemplate, EditorTemplateData{ - // EXTENDED to support printing editor in prompt + // EXTENDED to support printing editor in prompt, BlankAllowed Editor: *e.Editor, + BlankAllowed: e.BlankAllowed, EditorCommand: filepath.Base(e.editorCommand()), ShowHelp: true, Config: config, From 79ad1ed5d35d875745ad233e43fd791e517e8501 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 11 May 2020 17:39:52 -0500 Subject: [PATCH 22/23] tyop fix + stricter switch --- command/pr_review.go | 8 ++++++-- command/pr_review_test.go | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/command/pr_review.go b/command/pr_review.go index 3b01d0d5b35..d51aaff03ec 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -194,13 +194,17 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { return nil, err } - reviewState := api.ReviewComment + var reviewState api.PullRequestReviewState switch typeAnswers.ReviewType { case "Approve": reviewState = api.ReviewApprove - case "Request Changes": + case "Request changes": reviewState = api.ReviewRequestChanges + case "Comment": + reviewState = api.ReviewComment + default: + panic("unreachable state") } bodyAnswers := struct { diff --git a/command/pr_review_test.go b/command/pr_review_test.go index 231906ac080..c6c56c00d1d 100644 --- a/command/pr_review_test.go +++ b/command/pr_review_test.go @@ -315,7 +315,7 @@ func TestPRReview_interactive_no_body(t *testing.T) { as.Stub([]*QuestionStub{ { Name: "reviewType", - Value: "Request Changes", + Value: "Request changes", }, }) as.Stub([]*QuestionStub{ From c225d379a9599041b784c2be31e00380f66502a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 13 May 2020 12:09:59 +0200 Subject: [PATCH 23/23] Preserve CODEOWNERS reviewers in `pr create` When reviewers were requested on a PR, they would apparently overwrite the current set of reviewers. A fresh PR will already have reviewers if it was assigned some by CODEOWNERS rules. The fix is to only ever add additional reviewers and not overwrite the entire set. --- api/queries_pr.go | 1 + command/pr_create_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/api/queries_pr.go b/api/queries_pr.go index c1afc6bc9ef..c0b16f3c445 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -667,6 +667,7 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter requestReviews(input: $input) { clientMutationId } }` reviewParams["pullRequestId"] = pr.ID + reviewParams["union"] = true variables := map[string]interface{}{ "input": reviewParams, } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 85966aa7258..d6b89a6b051 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -184,6 +184,7 @@ func TestPRCreate_metadata(t *testing.T) { eq(t, inputs["pullRequestId"], "NEWPULLID") eq(t, inputs["userIds"], []interface{}{"HUBOTID"}) eq(t, inputs["teamIds"], []interface{}{"COREID"}) + eq(t, inputs["union"], true) })) cs, cmdTeardown := test.InitCmdStubber()