diff --git a/pkg/cmd/pr/diff/diff.go b/pkg/cmd/pr/diff/diff.go index b7ef11707..decfeb478 100644 --- a/pkg/cmd/pr/diff/diff.go +++ b/pkg/cmd/pr/diff/diff.go @@ -11,8 +11,10 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" @@ -22,6 +24,7 @@ import ( type DiffOptions struct { HttpClient func() (*http.Client, error) IO *iostreams.IOStreams + Browser browser.Browser Finder shared.PRFinder @@ -29,12 +32,14 @@ type DiffOptions struct { UseColor bool Patch bool NameOnly bool + BrowserMode bool } func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Command { opts := &DiffOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, + Browser: f.Browser, } var colorFlag string @@ -46,7 +51,9 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman View changes in a pull request. Without an argument, the pull request that belongs to the current branch - is selected. + is selected. + + With '--web', open the pull request diff in a web browser instead. `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { @@ -81,6 +88,7 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman cmdutil.StringEnumFlag(cmd, &colorFlag, "color", "", "auto", []string{"always", "never", "auto"}, "Use color in diff output") cmd.Flags().BoolVar(&opts.Patch, "patch", false, "Display diff in patch format") cmd.Flags().BoolVar(&opts.NameOnly, "name-only", false, "Display only names of changed files") + cmd.Flags().BoolVarP(&opts.BrowserMode, "web", "w", false, "Open the pull request diff in the browser") return cmd } @@ -90,11 +98,24 @@ func diffRun(opts *DiffOptions) error { Selector: opts.SelectorArg, Fields: []string{"number"}, } + + if opts.BrowserMode { + findOptions.Fields = []string{"url"} + } + pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { return err } + if opts.BrowserMode { + openUrl := fmt.Sprintf("%s/files", pr.URL) + if opts.IO.IsStdoutTTY() { + fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", text.DisplayURL(openUrl)) + } + return opts.Browser.Browse(openUrl) + } + httpClient, err := opts.HttpClient() if err != nil { return err diff --git a/pkg/cmd/pr/diff/diff_test.go b/pkg/cmd/pr/diff/diff_test.go index af0256e5b..bb8eb87bf 100644 --- a/pkg/cmd/pr/diff/diff_test.go +++ b/pkg/cmd/pr/diff/diff_test.go @@ -9,12 +9,12 @@ import ( "testing" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" - "github.com/google/go-cmp/cmp" "github.com/google/shlex" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -92,6 +92,16 @@ func Test_NewCmdDiff(t *testing.T) { isTTY: true, wantErr: "invalid argument \"doublerainbow\" for \"--color\" flag: valid values are {always|never|auto}", }, + { + name: "web mode", + args: "123 --web", + isTTY: true, + want: DiffOptions{ + SelectorArg: "123", + UseColor: true, + BrowserMode: true, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -130,19 +140,22 @@ func Test_NewCmdDiff(t *testing.T) { assert.Equal(t, tt.want.SelectorArg, opts.SelectorArg) assert.Equal(t, tt.want.UseColor, opts.UseColor) + assert.Equal(t, tt.want.BrowserMode, opts.BrowserMode) }) } } func Test_diffRun(t *testing.T) { - pr := &api.PullRequest{Number: 123} + pr := &api.PullRequest{Number: 123, URL: "https://github.com/OWNER/REPO/pull/123"} tests := []struct { - name string - opts DiffOptions - rawDiff string - wantAccept string - wantStdout string + name string + opts DiffOptions + wantFields []string + wantStdout string + wantStderr string + wantBrowsedURL string + httpStubs func(*httpmock.Registry) }{ { name: "no color", @@ -151,9 +164,11 @@ func Test_diffRun(t *testing.T) { UseColor: false, Patch: false, }, - rawDiff: fmt.Sprintf(testDiff, "", "", "", ""), - wantAccept: "application/vnd.github.v3.diff", + wantFields: []string{"number"}, wantStdout: fmt.Sprintf(testDiff, "", "", "", ""), + httpStubs: func(reg *httpmock.Registry) { + stubDiffRequest(reg, "application/vnd.github.v3.diff", fmt.Sprintf(testDiff, "", "", "", "")) + }, }, { name: "with color", @@ -162,9 +177,11 @@ func Test_diffRun(t *testing.T) { UseColor: true, Patch: false, }, - rawDiff: fmt.Sprintf(testDiff, "", "", "", ""), - wantAccept: "application/vnd.github.v3.diff", + wantFields: []string{"number"}, wantStdout: fmt.Sprintf(testDiff, "\x1b[m", "\x1b[1;38m", "\x1b[32m", "\x1b[31m"), + httpStubs: func(reg *httpmock.Registry) { + stubDiffRequest(reg, "application/vnd.github.v3.diff", fmt.Sprintf(testDiff, "", "", "", "")) + }, }, { name: "patch format", @@ -173,9 +190,11 @@ func Test_diffRun(t *testing.T) { UseColor: false, Patch: true, }, - rawDiff: fmt.Sprintf(testDiff, "", "", "", ""), - wantAccept: "application/vnd.github.v3.patch", + wantFields: []string{"number"}, wantStdout: fmt.Sprintf(testDiff, "", "", "", ""), + httpStubs: func(reg *httpmock.Registry) { + stubDiffRequest(reg, "application/vnd.github.v3.patch", fmt.Sprintf(testDiff, "", "", "", "")) + }, }, { name: "name only", @@ -185,51 +204,51 @@ func Test_diffRun(t *testing.T) { Patch: false, NameOnly: true, }, - rawDiff: fmt.Sprintf(testDiff, "", "", "", ""), - wantAccept: "application/vnd.github.v3.diff", + wantFields: []string{"number"}, wantStdout: ".github/workflows/releases.yml\nMakefile\n", + httpStubs: func(reg *httpmock.Registry) { + stubDiffRequest(reg, "application/vnd.github.v3.diff", fmt.Sprintf(testDiff, "", "", "", "")) + }, + }, + { + name: "web mode", + opts: DiffOptions{ + SelectorArg: "123", + BrowserMode: true, + }, + wantFields: []string{"url"}, + wantStderr: "Opening github.com/OWNER/REPO/pull/123/files in your browser.\n", + wantBrowsedURL: "https://github.com/OWNER/REPO/pull/123/files", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { httpReg := &httpmock.Registry{} defer httpReg.Verify(t) - - var gotAccept string - httpReg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/pulls/123"), - func(req *http.Request) (*http.Response, error) { - gotAccept = req.Header.Get("Accept") - return &http.Response{ - StatusCode: 200, - Request: req, - Body: io.NopCloser(strings.NewReader(tt.rawDiff)), - }, nil - }) - - opts := tt.opts - opts.HttpClient = func() (*http.Client, error) { + if tt.httpStubs != nil { + tt.httpStubs(httpReg) + } + tt.opts.HttpClient = func() (*http.Client, error) { return &http.Client{Transport: httpReg}, nil } - ios, _, stdout, stderr := iostreams.Test() - opts.IO = ios - finder := shared.NewMockFinder("123", pr, ghrepo.New("OWNER", "REPO")) - finder.ExpectFields([]string{"number"}) - opts.Finder = finder + browser := &browser.Stub{} + tt.opts.Browser = browser - if err := diffRun(&opts); err != nil { - t.Fatalf("unexpected error: %s", err) - } - if diff := cmp.Diff(tt.wantStdout, stdout.String()); diff != "" { - t.Errorf("command output did not match:\n%s", diff) - } - if stderr.String() != "" { - t.Errorf("unexpected stderr output: %s", stderr.String()) - } - if gotAccept != tt.wantAccept { - t.Errorf("unexpected Accept header: %s", gotAccept) - } + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(true) + tt.opts.IO = ios + + finder := shared.NewMockFinder("123", pr, ghrepo.New("OWNER", "REPO")) + finder.ExpectFields(tt.wantFields) + tt.opts.Finder = finder + + err := diffRun(&tt.opts) + assert.NoError(t, err) + + assert.Equal(t, tt.wantStdout, stdout.String()) + assert.Equal(t, tt.wantStderr, stderr.String()) + assert.Equal(t, tt.wantBrowsedURL, browser.BrowsedURL()) }) } } @@ -349,3 +368,23 @@ func Test_changedFileNames(t *testing.T) { } } } + +func stubDiffRequest(reg *httpmock.Registry, accept, diff string) { + reg.Register( + func(req *http.Request) bool { + if !strings.EqualFold(req.Method, "GET") { + return false + } + if req.URL.EscapedPath() != "/repos/OWNER/REPO/pulls/123" { + return false + } + return req.Header.Get("Accept") == accept + }, + func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: 200, + Request: req, + Body: io.NopCloser(strings.NewReader(diff)), + }, nil + }) +}