From 11466fda12b7ca98780b3b9981d232e2fd9e6e21 Mon Sep 17 00:00:00 2001 From: Siarhei Fedartsou Date: Sun, 12 Sep 2021 20:48:23 +0300 Subject: [PATCH 1/6] Add --draft and --non-draft filters to gh pr list --- pkg/cmd/pr/list/http.go | 12 ++++- pkg/cmd/pr/list/list.go | 12 ++++- pkg/cmd/pr/list/list_test.go | 91 ++++++++++++++++++++++++++++---- pkg/cmd/pr/shared/params.go | 7 +++ pkg/cmd/pr/shared/params_test.go | 28 ++++++++++ pkg/githubsearch/query.go | 8 +++ 6 files changed, 146 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/pr/list/http.go b/pkg/cmd/pr/list/http.go index 621853aaf..04ced2cca 100644 --- a/pkg/cmd/pr/list/http.go +++ b/pkg/cmd/pr/list/http.go @@ -10,8 +10,12 @@ import ( "github.com/cli/cli/v2/pkg/githubsearch" ) +func shouldUseSearch(filters prShared.FilterOptions) bool { + return filters.Draft || filters.NonDraft || filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 +} + func listPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters prShared.FilterOptions, limit int) (*api.PullRequestAndTotalCount, error) { - if filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 { + if shouldUseSearch(filters) { return searchPullRequests(httpClient, repo, filters, limit) } @@ -177,6 +181,12 @@ func searchPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters q.SetBaseBranch(filters.BaseBranch) } + if filters.Draft { + q.SetDraft(true) + } else if filters.NonDraft { + q.SetDraft(false) + } + pageLimit := min(limit, 100) variables := map[string]interface{}{ "q": q.String(), diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index 15c943541..4ef060bb7 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -37,6 +37,8 @@ type ListOptions struct { Author string Assignee string Search string + Draft bool + NonDraft bool } func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { @@ -74,6 +76,10 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman return &cmdutil.FlagError{Err: fmt.Errorf("invalid value for --limit: %v", opts.LimitResults)} } + if opts.Draft && opts.NonDraft { + return &cmdutil.FlagError{Err: fmt.Errorf("specify only one of `--draft` or `--non-draft`")} + } + if runF != nil { return runF(opts) } @@ -92,6 +98,9 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman cmd.Flags().StringVarP(&opts.Author, "author", "A", "", "Filter by author") cmd.Flags().StringVarP(&opts.Assignee, "assignee", "a", "", "Filter by assignee") cmd.Flags().StringVarP(&opts.Search, "search", "S", "", "Search pull requests with `query`") + cmd.Flags().BoolVar(&opts.Draft, "draft", false, "Show drafts only") + cmd.Flags().BoolVar(&opts.NonDraft, "non-draft", false, "Show non-drafts only") + cmdutil.AddJSONFlags(cmd, &opts.Exporter, api.PullRequestFields) return cmd @@ -132,12 +141,13 @@ func listRun(opts *ListOptions) error { Labels: opts.Labels, BaseBranch: opts.BaseBranch, Search: opts.Search, + Draft: opts.Draft, + NonDraft: opts.NonDraft, Fields: defaultFields, } if opts.Exporter != nil { filters.Fields = opts.Exporter.Fields() } - if opts.WebMode { prListURL := ghrepo.GenerateRepoURL(baseRepo, "pulls") openURL, err := shared.ListURLWithQuery(prListURL, filters) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index a912e4cb6..5d0086999 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -186,6 +186,52 @@ func TestPRList_filteringAssigneeLabels(t *testing.T) { } } +func TestPRList_bothNonDraftAndDraft(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + _, err := runCommand(http, true, `--draft --non-draft`) + if err == nil || err.Error() != "specify only one of `--draft` or `--non-draft`" { + t.Fatal(err) + } +} + +func TestPRList_filteringDraft(t *testing.T) { + tests := []struct { + name string + cli string + expectedQuery string + }{{ + "draft", + "--draft", + `repo:OWNER/REPO is:pr is:open draft:true`, + }, + { + "non-draft", + "--non-draft", + `repo:OWNER/REPO is:pr is:open draft:false`, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + http.Register( + httpmock.GraphQL(`query PullRequestSearch\b`), + httpmock.GraphQLQuery(`{}`, func(_ string, params map[string]interface{}) { + assert.Equal(t, test.expectedQuery, params["q"].(string)) + })) + + _, err := runCommand(http, true, test.cli) + if err != nil { + t.Fatal(err) + } + }) + } + +} + func TestPRList_withInvalidLimitFlag(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) @@ -197,18 +243,43 @@ func TestPRList_withInvalidLimitFlag(t *testing.T) { } func TestPRList_web(t *testing.T) { - http := initFakeHTTP() - defer http.Verify(t) + tests := []struct { + name string + cli string + expectedBrowserURL string + }{{ + "test", + "-a peter -l bug -l docs -L 10 -s merged -B trunk", + "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", + }, + { + "draft", + "--draft", + "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Atrue", + }, + { + "non-draft", + "--non-draft", + "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse", + }} - _, cmdTeardown := run.Stub() - defer cmdTeardown(t) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) - output, err := runCommand(http, true, "--web -a peter -l bug -l docs -L 10 -s merged -B trunk") - if err != nil { - t.Errorf("error running command `pr list` with `--web` flag: %v", err) + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + output, err := runCommand(http, true, "--web "+test.cli) + if err != nil { + t.Errorf("error running command `pr list` with `--web` flag: %v", err) + } + + assert.Equal(t, "", output.String()) + assert.Equal(t, "Opening github.com/OWNER/REPO/pulls in your browser.\n", output.Stderr()) + assert.Equal(t, test.expectedBrowserURL, output.BrowsedURL) + }) } - assert.Equal(t, "", output.String()) - assert.Equal(t, "Opening github.com/OWNER/REPO/pulls in your browser.\n", output.Stderr()) - assert.Equal(t, "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", output.BrowsedURL) } diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 92096e9d8..5e5ec4ef8 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -157,6 +157,8 @@ type FilterOptions struct { Mention string Milestone string Search string + Draft bool + NonDraft bool Fields []string } @@ -241,6 +243,11 @@ func SearchQueryBuild(options FilterOptions) string { if options.Search != "" { q.AddQuery(options.Search) } + if options.Draft { + q.SetDraft(true) + } else if options.NonDraft { + q.SetDraft(false) + } return q.String() } diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index fa41ac307..696950722 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -34,6 +34,34 @@ func Test_listURLWithQuery(t *testing.T) { want: "https://example.com/path?a=b&q=is%3Aissue+is%3Aopen", wantErr: false, }, + { + name: "draft", + args: args{ + listURL: "https://example.com/path", + options: FilterOptions{ + Entity: "pr", + State: "open", + Draft: true, + NonDraft: true, // shouldn't impact anything - we always prefer `draft` + }, + }, + want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Atrue", + wantErr: false, + }, + { + name: "non-draft", + args: args{ + listURL: "https://example.com/path", + options: FilterOptions{ + Entity: "pr", + State: "open", + Draft: false, + NonDraft: true, + }, + }, + want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Afalse", + wantErr: false, + }, { name: "all", args: args{ diff --git a/pkg/githubsearch/query.go b/pkg/githubsearch/query.go index f25707710..b0938613a 100644 --- a/pkg/githubsearch/query.go +++ b/pkg/githubsearch/query.go @@ -54,6 +54,7 @@ type Query struct { forkState string visibility string isArchived *bool + isDraft *bool } func (q *Query) InRepository(nameWithOwner string) { @@ -139,6 +140,10 @@ func (q *Query) SetArchived(isArchived bool) { q.isArchived = &isArchived } +func (q *Query) SetDraft(isDraft bool) { + q.isDraft = &isDraft +} + func (q *Query) String() string { var qs string @@ -198,6 +203,9 @@ func (q *Query) String() string { if q.headBranch != "" { qs += fmt.Sprintf("head:%s ", quote(q.headBranch)) } + if q.isDraft != nil { + qs += fmt.Sprintf("draft:%v ", *q.isDraft) + } if q.sort != "" { qs += fmt.Sprintf("sort:%s ", q.sort) From 373d1efb583bc65a71ba3838ed4d60e6aaa6718f Mon Sep 17 00:00:00 2001 From: Siarhei Fedartsou Date: Sun, 12 Sep 2021 20:50:21 +0300 Subject: [PATCH 2/6] format code --- pkg/cmd/pr/list/list_test.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 5d0086999..1823175c7 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -201,11 +201,12 @@ func TestPRList_filteringDraft(t *testing.T) { name string cli string expectedQuery string - }{{ - "draft", - "--draft", - `repo:OWNER/REPO is:pr is:open draft:true`, - }, + }{ + { + "draft", + "--draft", + `repo:OWNER/REPO is:pr is:open draft:true`, + }, { "non-draft", "--non-draft", @@ -247,11 +248,12 @@ func TestPRList_web(t *testing.T) { name string cli string expectedBrowserURL string - }{{ - "test", - "-a peter -l bug -l docs -L 10 -s merged -B trunk", - "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", - }, + }{ + { + "test", + "-a peter -l bug -l docs -L 10 -s merged -B trunk", + "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", + }, { "draft", "--draft", From f3053c36287078aa144ff7ac259ac5d89de0a3e0 Mon Sep 17 00:00:00 2001 From: Siarhei Fedartsou Date: Sun, 12 Sep 2021 20:58:13 +0300 Subject: [PATCH 3/6] fix tests --- pkg/cmd/pr/list/list_test.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 1823175c7..2d3aa62aa 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -191,9 +191,8 @@ func TestPRList_bothNonDraftAndDraft(t *testing.T) { defer http.Verify(t) _, err := runCommand(http, true, `--draft --non-draft`) - if err == nil || err.Error() != "specify only one of `--draft` or `--non-draft`" { - t.Fatal(err) - } + assert.NotNil(t, err) + assert.Equal(t, err.Error(), "specify only one of `--draft` or `--non-draft`") } func TestPRList_filteringDraft(t *testing.T) { @@ -238,9 +237,8 @@ func TestPRList_withInvalidLimitFlag(t *testing.T) { defer http.Verify(t) _, err := runCommand(http, true, `--limit=0`) - if err == nil && err.Error() != "invalid limit: 0" { - t.Errorf("error running command `issue list`: %v", err) - } + assert.NotNil(t, err) + assert.Equal(t, err.Error(), "invalid value for --limit: 0") } func TestPRList_web(t *testing.T) { From 1926971a762325d3f8a46b08efcb54770ab2aa8c Mon Sep 17 00:00:00 2001 From: Siarhei Fedartsou Date: Sun, 12 Sep 2021 21:04:03 +0300 Subject: [PATCH 4/6] Remove non-relevant test --- pkg/cmd/pr/list/list_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 2d3aa62aa..700a03f08 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -176,16 +176,6 @@ func TestPRList_filteringAssignee(t *testing.T) { } } -func TestPRList_filteringAssigneeLabels(t *testing.T) { - http := initFakeHTTP() - defer http.Verify(t) - - _, err := runCommand(http, true, `-l one,two -a hubot`) - if err == nil && err.Error() != "multiple labels with --assignee are not supported" { - t.Fatal(err) - } -} - func TestPRList_bothNonDraftAndDraft(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) From 6a34f53c6c50046cfbe47eb681208fc89f83e3e5 Mon Sep 17 00:00:00 2001 From: Siarhei Fedartsou Date: Fri, 17 Sep 2021 22:05:47 +0300 Subject: [PATCH 5/6] Change pr list --draft UX --- pkg/cmd/pr/list/http.go | 8 +++----- pkg/cmd/pr/list/list.go | 13 ++++++------- pkg/cmd/pr/list/list_test.go | 20 ++++++-------------- pkg/cmd/pr/shared/params.go | 11 ++++------- pkg/cmd/pr/shared/params_test.go | 17 +++++++++-------- 5 files changed, 28 insertions(+), 41 deletions(-) diff --git a/pkg/cmd/pr/list/http.go b/pkg/cmd/pr/list/http.go index 04ced2cca..8f77b8023 100644 --- a/pkg/cmd/pr/list/http.go +++ b/pkg/cmd/pr/list/http.go @@ -11,7 +11,7 @@ import ( ) func shouldUseSearch(filters prShared.FilterOptions) bool { - return filters.Draft || filters.NonDraft || filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 + return filters.Draft != nil || filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 } func listPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters prShared.FilterOptions, limit int) (*api.PullRequestAndTotalCount, error) { @@ -181,10 +181,8 @@ func searchPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters q.SetBaseBranch(filters.BaseBranch) } - if filters.Draft { - q.SetDraft(true) - } else if filters.NonDraft { - q.SetDraft(false) + if filters.Draft != nil { + q.SetDraft(*filters.Draft) } pageLimit := min(limit, 100) diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index 4ef060bb7..2e9056e88 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -37,15 +37,16 @@ type ListOptions struct { Author string Assignee string Search string - Draft bool - NonDraft bool + Draft *bool } func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { + draft := false opts := &ListOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, Browser: f.Browser, + Draft: &draft, } cmd := &cobra.Command{ @@ -76,8 +77,8 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman return &cmdutil.FlagError{Err: fmt.Errorf("invalid value for --limit: %v", opts.LimitResults)} } - if opts.Draft && opts.NonDraft { - return &cmdutil.FlagError{Err: fmt.Errorf("specify only one of `--draft` or `--non-draft`")} + if !cmd.Flags().Changed("draft") { + opts.Draft = nil } if runF != nil { @@ -98,8 +99,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman cmd.Flags().StringVarP(&opts.Author, "author", "A", "", "Filter by author") cmd.Flags().StringVarP(&opts.Assignee, "assignee", "a", "", "Filter by assignee") cmd.Flags().StringVarP(&opts.Search, "search", "S", "", "Search pull requests with `query`") - cmd.Flags().BoolVar(&opts.Draft, "draft", false, "Show drafts only") - cmd.Flags().BoolVar(&opts.NonDraft, "non-draft", false, "Show non-drafts only") + cmd.Flags().BoolVar(opts.Draft, "draft", false, "Filter by draft/non-draft") cmdutil.AddJSONFlags(cmd, &opts.Exporter, api.PullRequestFields) @@ -142,7 +142,6 @@ func listRun(opts *ListOptions) error { BaseBranch: opts.BaseBranch, Search: opts.Search, Draft: opts.Draft, - NonDraft: opts.NonDraft, Fields: defaultFields, } if opts.Exporter != nil { diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 700a03f08..9043d9274 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -176,15 +176,6 @@ func TestPRList_filteringAssignee(t *testing.T) { } } -func TestPRList_bothNonDraftAndDraft(t *testing.T) { - http := initFakeHTTP() - defer http.Verify(t) - - _, err := runCommand(http, true, `--draft --non-draft`) - assert.NotNil(t, err) - assert.Equal(t, err.Error(), "specify only one of `--draft` or `--non-draft`") -} - func TestPRList_filteringDraft(t *testing.T) { tests := []struct { name string @@ -193,12 +184,12 @@ func TestPRList_filteringDraft(t *testing.T) { }{ { "draft", - "--draft", + "--draft=1", `repo:OWNER/REPO is:pr is:open draft:true`, }, { "non-draft", - "--non-draft", + "--draft=false", `repo:OWNER/REPO is:pr is:open draft:false`, }} @@ -244,14 +235,15 @@ func TestPRList_web(t *testing.T) { }, { "draft", - "--draft", + "--draft=true", "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Atrue", }, { "non-draft", - "--non-draft", + "--draft=0", "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse", - }} + }, + } for _, test := range tests { t.Run(test.name, func(t *testing.T) { diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 5e5ec4ef8..846a05022 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -157,8 +157,8 @@ type FilterOptions struct { Mention string Milestone string Search string - Draft bool - NonDraft bool + // filter by draft/non-draft, `nil` means "no filter" + Draft *bool Fields []string } @@ -243,12 +243,9 @@ func SearchQueryBuild(options FilterOptions) string { if options.Search != "" { q.AddQuery(options.Search) } - if options.Draft { - q.SetDraft(true) - } else if options.NonDraft { - q.SetDraft(false) + if options.Draft != nil { + q.SetDraft(*options.Draft) } - return q.String() } diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index 696950722..a40916077 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -16,6 +16,9 @@ func Test_listURLWithQuery(t *testing.T) { listURL string options FilterOptions } + draft := true + noDraft := false + tests := []struct { name string args args @@ -39,10 +42,9 @@ func Test_listURLWithQuery(t *testing.T) { args: args{ listURL: "https://example.com/path", options: FilterOptions{ - Entity: "pr", - State: "open", - Draft: true, - NonDraft: true, // shouldn't impact anything - we always prefer `draft` + Entity: "pr", + State: "open", + Draft: &draft, }, }, want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Atrue", @@ -53,10 +55,9 @@ func Test_listURLWithQuery(t *testing.T) { args: args{ listURL: "https://example.com/path", options: FilterOptions{ - Entity: "pr", - State: "open", - Draft: false, - NonDraft: true, + Entity: "pr", + State: "open", + Draft: &noDraft, }, }, want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Afalse", From d14715f1e37e7db19408dea8cc3162f8b2c3fb9f Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Mon, 20 Sep 2021 11:29:37 -0700 Subject: [PATCH 6/6] Convert bool to string early for pr list draft flag --- pkg/cmd/pr/list/http.go | 6 ++--- pkg/cmd/pr/list/list.go | 12 +++++----- pkg/cmd/pr/list/list_test.go | 39 +++++++++++++++----------------- pkg/cmd/pr/shared/params.go | 10 ++++---- pkg/cmd/pr/shared/params_test.go | 6 ++--- pkg/githubsearch/query.go | 10 ++++---- 6 files changed, 38 insertions(+), 45 deletions(-) diff --git a/pkg/cmd/pr/list/http.go b/pkg/cmd/pr/list/http.go index 8f77b8023..43e87b7fd 100644 --- a/pkg/cmd/pr/list/http.go +++ b/pkg/cmd/pr/list/http.go @@ -11,7 +11,7 @@ import ( ) func shouldUseSearch(filters prShared.FilterOptions) bool { - return filters.Draft != nil || filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 + return filters.Draft != "" || filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 } func listPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters prShared.FilterOptions, limit int) (*api.PullRequestAndTotalCount, error) { @@ -181,8 +181,8 @@ func searchPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters q.SetBaseBranch(filters.BaseBranch) } - if filters.Draft != nil { - q.SetDraft(*filters.Draft) + if filters.Draft != "" { + q.SetDraft(filters.Draft) } pageLimit := min(limit, 100) diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index 2e9056e88..76013fd88 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -37,18 +37,18 @@ type ListOptions struct { Author string Assignee string Search string - Draft *bool + Draft string } func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { - draft := false opts := &ListOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, Browser: f.Browser, - Draft: &draft, } + var draft bool + cmd := &cobra.Command{ Use: "list", Short: "List and filter pull requests in this repository", @@ -77,8 +77,8 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman return &cmdutil.FlagError{Err: fmt.Errorf("invalid value for --limit: %v", opts.LimitResults)} } - if !cmd.Flags().Changed("draft") { - opts.Draft = nil + if cmd.Flags().Changed("draft") { + opts.Draft = strconv.FormatBool(draft) } if runF != nil { @@ -99,7 +99,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman cmd.Flags().StringVarP(&opts.Author, "author", "A", "", "Filter by author") cmd.Flags().StringVarP(&opts.Assignee, "assignee", "a", "", "Filter by assignee") cmd.Flags().StringVarP(&opts.Search, "search", "S", "", "Search pull requests with `query`") - cmd.Flags().BoolVar(opts.Draft, "draft", false, "Filter by draft/non-draft") + cmd.Flags().BoolVarP(&draft, "draft", "d", false, "Filter by draft state") cmdutil.AddJSONFlags(cmd, &opts.Exporter, api.PullRequestFields) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 9043d9274..ff55089f0 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -183,15 +183,16 @@ func TestPRList_filteringDraft(t *testing.T) { expectedQuery string }{ { - "draft", - "--draft=1", - `repo:OWNER/REPO is:pr is:open draft:true`, + name: "draft", + cli: "--draft", + expectedQuery: `repo:OWNER/REPO is:pr is:open draft:true`, }, { - "non-draft", - "--draft=false", - `repo:OWNER/REPO is:pr is:open draft:false`, - }} + name: "non-draft", + cli: "--draft=false", + expectedQuery: `repo:OWNER/REPO is:pr is:open draft:false`, + }, + } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -210,16 +211,13 @@ func TestPRList_filteringDraft(t *testing.T) { } }) } - } func TestPRList_withInvalidLimitFlag(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - _, err := runCommand(http, true, `--limit=0`) - assert.NotNil(t, err) - assert.Equal(t, err.Error(), "invalid value for --limit: 0") + assert.EqualError(t, err, "invalid value for --limit: 0") } func TestPRList_web(t *testing.T) { @@ -229,19 +227,19 @@ func TestPRList_web(t *testing.T) { expectedBrowserURL string }{ { - "test", - "-a peter -l bug -l docs -L 10 -s merged -B trunk", - "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", + name: "filters", + cli: "-a peter -l bug -l docs -L 10 -s merged -B trunk", + expectedBrowserURL: "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", }, { - "draft", - "--draft=true", - "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Atrue", + name: "draft", + cli: "--draft=true", + expectedBrowserURL: "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Atrue", }, { - "non-draft", - "--draft=0", - "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse", + name: "non-draft", + cli: "--draft=0", + expectedBrowserURL: "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse", }, } @@ -263,5 +261,4 @@ func TestPRList_web(t *testing.T) { assert.Equal(t, test.expectedBrowserURL, output.BrowsedURL) }) } - } diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 846a05022..2333bcfa3 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -157,10 +157,8 @@ type FilterOptions struct { Mention string Milestone string Search string - // filter by draft/non-draft, `nil` means "no filter" - Draft *bool - - Fields []string + Draft string + Fields []string } func (opts *FilterOptions) IsDefault() bool { @@ -243,8 +241,8 @@ func SearchQueryBuild(options FilterOptions) string { if options.Search != "" { q.AddQuery(options.Search) } - if options.Draft != nil { - q.SetDraft(*options.Draft) + if options.Draft != "" { + q.SetDraft(options.Draft) } return q.String() } diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index a40916077..8f3e793e5 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -16,8 +16,6 @@ func Test_listURLWithQuery(t *testing.T) { listURL string options FilterOptions } - draft := true - noDraft := false tests := []struct { name string @@ -44,7 +42,7 @@ func Test_listURLWithQuery(t *testing.T) { options: FilterOptions{ Entity: "pr", State: "open", - Draft: &draft, + Draft: "true", }, }, want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Atrue", @@ -57,7 +55,7 @@ func Test_listURLWithQuery(t *testing.T) { options: FilterOptions{ Entity: "pr", State: "open", - Draft: &noDraft, + Draft: "false", }, }, want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Afalse", diff --git a/pkg/githubsearch/query.go b/pkg/githubsearch/query.go index b0938613a..a8f3005a9 100644 --- a/pkg/githubsearch/query.go +++ b/pkg/githubsearch/query.go @@ -54,7 +54,7 @@ type Query struct { forkState string visibility string isArchived *bool - isDraft *bool + draft string } func (q *Query) InRepository(nameWithOwner string) { @@ -140,8 +140,8 @@ func (q *Query) SetArchived(isArchived bool) { q.isArchived = &isArchived } -func (q *Query) SetDraft(isDraft bool) { - q.isDraft = &isDraft +func (q *Query) SetDraft(draft string) { + q.draft = draft } func (q *Query) String() string { @@ -203,8 +203,8 @@ func (q *Query) String() string { if q.headBranch != "" { qs += fmt.Sprintf("head:%s ", quote(q.headBranch)) } - if q.isDraft != nil { - qs += fmt.Sprintf("draft:%v ", *q.isDraft) + if q.draft != "" { + qs += fmt.Sprintf("draft:%v ", q.draft) } if q.sort != "" {