From b94860bae7b25f384de7a084d314c470e8616279 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 15:43:27 -0500 Subject: [PATCH 1/8] add command file with pr resolution --- command/pr_diff.go | 75 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 command/pr_diff.go diff --git a/command/pr_diff.go b/command/pr_diff.go new file mode 100644 index 000000000..db44bdef3 --- /dev/null +++ b/command/pr_diff.go @@ -0,0 +1,75 @@ +package command + +import ( + "errors" + "fmt" + "strconv" + "strings" + + "github.com/cli/cli/api" + "github.com/spf13/cobra" +) + +var prDiffCmd = &cobra.Command{ + Use: "diff { | }", + Short: "View a pull request's changes.", + RunE: prDiff, +} + +func init() { + prDiffCmd.Flags().StringP("color", "c", "auto", "Whether or not to output color: {always|never|auto}") + + prCmd.AddCommand(prDiffCmd) +} + +func prDiff(cmd *cobra.Command, args []string) error { + ctx := contextForCommand(cmd) + apiClient, err := apiClientForContext(ctx) + if err != nil { + return err + } + + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) + if err != nil { + return fmt.Errorf("could not determine base repo: %w", err) + } + + var prNum int + branchWithOwner := "" + + if len(args) == 0 { + prNum, branchWithOwner, err = prSelectorForCurrentBranch(ctx, baseRepo) + if err != nil { + return fmt.Errorf("could not query for pull request for current branch: %w", err) + } + } else { + prArg, repo := prFromURL(args[0]) + if repo != nil { + baseRepo = repo + } else { + prArg = strings.TrimPrefix(args[0], "#") + } + prNum, err = strconv.Atoi(prArg) + if err != nil { + return errors.New("could not parse pull request argument") + } + } + + var pr *api.PullRequest + if prNum > 0 { + pr, err = api.PullRequestByNumber(apiClient, baseRepo, prNum) + if err != nil { + return fmt.Errorf("could not find pull request: %w", err) + } + } else { + pr, err = api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) + if err != nil { + return fmt.Errorf("could not find pull request: %w", err) + } + prNum = pr.Number + } + + fmt.Println(pr.Title) + + return nil +} From aaebdfc46f1da12868560fed327333923a80b6bc Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 16:49:51 -0500 Subject: [PATCH 2/8] working with gross colorize hack + no pager --- api/queries_pr.go | 26 ++++++++++++++++++++++++++ command/pr_diff.go | 41 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index c0b16f3c4..80428ae07 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -3,6 +3,8 @@ package api import ( "context" "fmt" + "io/ioutil" + "net/http" "strings" "github.com/shurcooL/githubv4" @@ -201,6 +203,30 @@ func (pr *PullRequest) ChecksStatus() (summary PullRequestChecksStatus) { return } +func (c Client) PullRequestDiff(baseRepo ghrepo.Interface, prNum int) (string, error) { + url := fmt.Sprintf("https://api.github.com/repos/%s/pulls/%d", + ghrepo.FullName(baseRepo), prNum) + req, err := http.NewRequest("GET", url, nil) + if err != nil { + return "", err + } + + req.Header.Set("Accept", "application/vnd.github.v3.diff; charset=utf-8") + + resp, err := c.http.Do(req) + if err != nil { + return "", err + } + defer resp.Body.Close() + + b, err := ioutil.ReadAll(resp.Body) + if err != nil { + return "", err + } + + return string(b), nil +} + func PullRequests(client *Client, repo ghrepo.Interface, currentPRNumber int, currentPRHeadRef, currentUsername string) (*PullRequestsPayload, error) { type edges struct { TotalCount int diff --git a/command/pr_diff.go b/command/pr_diff.go index db44bdef3..56e52f551 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -3,10 +3,12 @@ package command import ( "errors" "fmt" + "os" "strconv" "strings" "github.com/cli/cli/api" + "github.com/cli/cli/utils" "github.com/spf13/cobra" ) @@ -34,6 +36,7 @@ func prDiff(cmd *cobra.Command, args []string) error { return fmt.Errorf("could not determine base repo: %w", err) } + // begin pr resolution boilerplate var prNum int branchWithOwner := "" @@ -68,8 +71,44 @@ func prDiff(cmd *cobra.Command, args []string) error { } prNum = pr.Number } + // end pr resolution boilerplate - fmt.Println(pr.Title) + diff, err := apiClient.PullRequestDiff(baseRepo, prNum) + if err != nil { + return err + } + + color, err := cmd.Flags().GetString("color") + if err != nil { + return err + } + + out := cmd.OutOrStdout() + if color == "auto" { + color = "never" + isTTY := false + if outFile, isFile := out.(*os.File); isFile { + isTTY = utils.IsTerminal(outFile) + if isTTY { + color = "always" + } + } + } + + switch color { + case "always": + out = colorableOut(cmd) + rendered, err := utils.RenderMarkdown(fmt.Sprintf("```diff\n%s\n```", diff)) + fmt.Fprintf(out, rendered) + if err != nil { + return fmt.Errorf("failed to colorize diff: %w", err) + } + case "never": + out := cmd.OutOrStdout() + fmt.Fprintf(out, diff) + default: + return fmt.Errorf("did not understand color setting %q", color) + } return nil } From c159d41fc205e2295b018fdebe97fdf5e098b03e Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 17:15:48 -0500 Subject: [PATCH 3/8] manually colorize --- command/pr_diff.go | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/command/pr_diff.go b/command/pr_diff.go index 56e52f551..90cc608f3 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -98,10 +98,18 @@ func prDiff(cmd *cobra.Command, args []string) error { switch color { case "always": out = colorableOut(cmd) - rendered, err := utils.RenderMarkdown(fmt.Sprintf("```diff\n%s\n```", diff)) - fmt.Fprintf(out, rendered) - if err != nil { - return fmt.Errorf("failed to colorize diff: %w", err) + for _, diffLine := range strings.Split(diff, "\n") { + output := diffLine + switch { + case bold(diffLine): + output = utils.Bold(diffLine) + case green(diffLine): + output = utils.Green(diffLine) + case red(diffLine): + output = utils.Red(diffLine) + } + + fmt.Fprintln(out, output) } case "never": out := cmd.OutOrStdout() @@ -112,3 +120,21 @@ func prDiff(cmd *cobra.Command, args []string) error { return nil } + +func bold(dl string) bool { + prefixes := []string{"+++", "---", "diff", "index"} + for _, p := range prefixes { + if strings.HasPrefix(dl, p) { + return true + } + } + return false +} + +func red(dl string) bool { + return strings.HasPrefix(dl, "-") +} + +func green(dl string) bool { + return strings.HasPrefix(dl, "+") +} From 8e6b8d39014cd691f96ff76b7640735283a8e25f Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 17:23:36 -0500 Subject: [PATCH 4/8] cleanup --- command/pr_diff.go | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/command/pr_diff.go b/command/pr_diff.go index 90cc608f3..622b50edd 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -82,6 +82,9 @@ func prDiff(cmd *cobra.Command, args []string) error { if err != nil { return err } + if !validColor(color) { + return fmt.Errorf("did not understand color: %q. Expected one of always, never, or auto", color) + } out := cmd.OutOrStdout() if color == "auto" { @@ -95,27 +98,24 @@ func prDiff(cmd *cobra.Command, args []string) error { } } - switch color { - case "always": - out = colorableOut(cmd) - for _, diffLine := range strings.Split(diff, "\n") { - output := diffLine - switch { - case bold(diffLine): - output = utils.Bold(diffLine) - case green(diffLine): - output = utils.Green(diffLine) - case red(diffLine): - output = utils.Red(diffLine) - } + if color == "never" { + fmt.Fprint(out, diff) + return nil + } - fmt.Fprintln(out, output) + out = colorableOut(cmd) + for _, diffLine := range strings.Split(diff, "\n") { + output := diffLine + switch { + case bold(diffLine): + output = utils.Bold(diffLine) + case green(diffLine): + output = utils.Green(diffLine) + case red(diffLine): + output = utils.Red(diffLine) } - case "never": - out := cmd.OutOrStdout() - fmt.Fprintf(out, diff) - default: - return fmt.Errorf("did not understand color setting %q", color) + + fmt.Fprintln(out, output) } return nil @@ -138,3 +138,7 @@ func red(dl string) bool { func green(dl string) bool { return strings.HasPrefix(dl, "+") } + +func validColor(c string) bool { + return c == "auto" || c == "always" || c == "never" +} From 51b421207054e39c286d0a2d4db0c4b38e8550d8 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 17:39:54 -0500 Subject: [PATCH 5/8] add basic tests --- command/pr_diff.go | 16 +++++----- command/pr_diff_test.go | 67 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 8 deletions(-) create mode 100644 command/pr_diff_test.go diff --git a/command/pr_diff.go b/command/pr_diff.go index 622b50edd..f311fcc7a 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -25,6 +25,14 @@ func init() { } func prDiff(cmd *cobra.Command, args []string) error { + color, err := cmd.Flags().GetString("color") + if err != nil { + return err + } + if !validColor(color) { + return fmt.Errorf("did not understand color: %q. Expected one of always, never, or auto", color) + } + ctx := contextForCommand(cmd) apiClient, err := apiClientForContext(ctx) if err != nil { @@ -78,14 +86,6 @@ func prDiff(cmd *cobra.Command, args []string) error { return err } - color, err := cmd.Flags().GetString("color") - if err != nil { - return err - } - if !validColor(color) { - return fmt.Errorf("did not understand color: %q. Expected one of always, never, or auto", color) - } - out := cmd.OutOrStdout() if color == "auto" { color = "never" diff --git a/command/pr_diff_test.go b/command/pr_diff_test.go new file mode 100644 index 000000000..a3705dad6 --- /dev/null +++ b/command/pr_diff_test.go @@ -0,0 +1,67 @@ +package command + +import ( + "bytes" + "testing" +) + +func TestPRDiff_validation(t *testing.T) { + _, err := RunCommand("pr diff --color=doublerainbow") + if err == nil { + t.Fatal("expected error") + } + eq(t, err.Error(), `did not understand color: "doublerainbow". Expected one of always, never, or auto`) +} + +func TestPRDiff(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", + "number": 123, + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } }`)) + http.StubResponse(200, bytes.NewBufferString(testDiff)) + output, err := RunCommand("pr diff") + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + eq(t, testDiff, output.String()) +} + +const testDiff = `diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml +index 73974448..b7fc0154 100644 +--- a/.github/workflows/releases.yml ++++ b/.github/workflows/releases.yml +@@ -44,6 +44,11 @@ jobs: + token: ${{secrets.SITE_GITHUB_TOKEN}} + - name: Publish documentation site + if: "!contains(github.ref, '-')" # skip prereleases ++ env: ++ GIT_COMMITTER_NAME: cli automation ++ GIT_AUTHOR_NAME: cli automation ++ GIT_COMMITTER_EMAIL: noreply@github.com ++ GIT_AUTHOR_EMAIL: noreply@github.com + run: make site-publish + - name: Move project cards + if: "!contains(github.ref, '-')" # skip prereleases +diff --git a/Makefile b/Makefile +index f2b4805c..3d7bd0f9 100644 +--- a/Makefile ++++ b/Makefile +@@ -22,8 +22,8 @@ test: + go test ./... + .PHONY: test + +-site: +- git clone https://github.com/github/cli.github.com.git "$@" ++site: bin/gh ++ bin/gh repo clone github/cli.github.com "$@" + + site-docs: site + git -C site pull +` From ccda5ced5136b50f18a336a20d1f0727d5b2737c Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 17:45:47 -0500 Subject: [PATCH 6/8] lint --- command/pr_diff.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/command/pr_diff.go b/command/pr_diff.go index f311fcc7a..a6dccdd12 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -66,14 +66,8 @@ func prDiff(cmd *cobra.Command, args []string) error { } } - var pr *api.PullRequest - if prNum > 0 { - pr, err = api.PullRequestByNumber(apiClient, baseRepo, prNum) - if err != nil { - return fmt.Errorf("could not find pull request: %w", err) - } - } else { - pr, err = api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) + if prNum < 1 { + pr, err := api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) if err != nil { return fmt.Errorf("could not find pull request: %w", err) } From c98b0924dcf03fcb465750e3ab9b859c9240f524 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 19 May 2020 11:58:49 -0500 Subject: [PATCH 7/8] properly handle REST errors --- api/queries_pr.go | 12 +++++++++++- command/pr_diff.go | 2 +- command/pr_diff_test.go | 34 +++++++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 80428ae07..26c88af01 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -2,6 +2,7 @@ package api import ( "context" + "errors" "fmt" "io/ioutil" "net/http" @@ -224,7 +225,16 @@ func (c Client) PullRequestDiff(baseRepo ghrepo.Interface, prNum int) (string, e return "", err } - return string(b), nil + if resp.StatusCode == 200 { + return string(b), nil + } + + if resp.StatusCode == 404 { + return "", &NotFoundError{errors.New("pull request not found")} + } + + return "", errors.New("pull request diff lookup failed") + } func PullRequests(client *Client, repo ghrepo.Interface, currentPRNumber int, currentPRHeadRef, currentUsername string) (*PullRequestsPayload, error) { diff --git a/command/pr_diff.go b/command/pr_diff.go index a6dccdd12..6f48716da 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -77,7 +77,7 @@ func prDiff(cmd *cobra.Command, args []string) error { diff, err := apiClient.PullRequestDiff(baseRepo, prNum) if err != nil { - return err + return fmt.Errorf("could not find pull request diff: %w", err) } out := cmd.OutOrStdout() diff --git a/command/pr_diff_test.go b/command/pr_diff_test.go index a3705dad6..ab883dcab 100644 --- a/command/pr_diff_test.go +++ b/command/pr_diff_test.go @@ -13,6 +13,38 @@ func TestPRDiff_validation(t *testing.T) { eq(t, err.Error(), `did not understand color: "doublerainbow". Expected one of always, never, or auto`) } +func TestPRDiff_no_current_pr(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + 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(testDiff)) + _, err := RunCommand("pr diff") + if err == nil { + t.Fatal("expected error", err) + } + eq(t, err.Error(), `could not find pull request: no open pull requests found for branch "master"`) +} + +func TestPRDiff_argument_not_found(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(404, bytes.NewBufferString("")) + _, err := RunCommand("pr diff 123") + if err == nil { + t.Fatal("expected error", err) + } + eq(t, err.Error(), `could not find pull request diff: pull request not found`) +} + func TestPRDiff(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") http := initFakeHTTP() @@ -30,7 +62,7 @@ func TestPRDiff(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %s", err) } - eq(t, testDiff, output.String()) + eq(t, output.String(), testDiff) } const testDiff = `diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml From 983a1d9c3c6690503c604389dfe5dca728d70561 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 19 May 2020 14:08:20 -0500 Subject: [PATCH 8/8] better names --- command/pr_diff.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/command/pr_diff.go b/command/pr_diff.go index 6f48716da..6e4bb4bdb 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -29,7 +29,7 @@ func prDiff(cmd *cobra.Command, args []string) error { if err != nil { return err } - if !validColor(color) { + if !validColorFlag(color) { return fmt.Errorf("did not understand color: %q. Expected one of always, never, or auto", color) } @@ -101,11 +101,11 @@ func prDiff(cmd *cobra.Command, args []string) error { for _, diffLine := range strings.Split(diff, "\n") { output := diffLine switch { - case bold(diffLine): + case isHeaderLine(diffLine): output = utils.Bold(diffLine) - case green(diffLine): + case isAdditionLine(diffLine): output = utils.Green(diffLine) - case red(diffLine): + case isRemovalLine(diffLine): output = utils.Red(diffLine) } @@ -115,7 +115,7 @@ func prDiff(cmd *cobra.Command, args []string) error { return nil } -func bold(dl string) bool { +func isHeaderLine(dl string) bool { prefixes := []string{"+++", "---", "diff", "index"} for _, p := range prefixes { if strings.HasPrefix(dl, p) { @@ -125,14 +125,14 @@ func bold(dl string) bool { return false } -func red(dl string) bool { - return strings.HasPrefix(dl, "-") -} - -func green(dl string) bool { +func isAdditionLine(dl string) bool { return strings.HasPrefix(dl, "+") } -func validColor(c string) bool { +func isRemovalLine(dl string) bool { + return strings.HasPrefix(dl, "-") +} + +func validColorFlag(c string) bool { return c == "auto" || c == "always" || c == "never" }