From 6a34f53c6c50046cfbe47eb681208fc89f83e3e5 Mon Sep 17 00:00:00 2001 From: Siarhei Fedartsou Date: Fri, 17 Sep 2021 22:05:47 +0300 Subject: [PATCH] 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",