diff --git a/pkg/cmd/discussion/client/client.go b/pkg/cmd/discussion/client/client.go index b698dc896..7f5fdfd4b 100644 --- a/pkg/cmd/discussion/client/client.go +++ b/pkg/cmd/discussion/client/client.go @@ -9,8 +9,8 @@ import "github.com/cli/cli/v2/internal/ghrepo" // DiscussionClient defines operations for interacting with the GitHub Discussions API. type DiscussionClient interface { - List(repo ghrepo.Interface, filters ListFilters, limit int) ([]Discussion, int, error) - Search(repo ghrepo.Interface, filters SearchFilters, limit int) ([]Discussion, int, error) + List(repo ghrepo.Interface, filters ListFilters, after string, limit int) (DiscussionListResult, error) + Search(repo ghrepo.Interface, filters SearchFilters, after string, limit int) (DiscussionListResult, error) GetByNumber(repo ghrepo.Interface, number int) (*Discussion, error) GetWithComments(repo ghrepo.Interface, number int, commentLimit int, order string) (*Discussion, error) ListCategories(repo ghrepo.Interface) ([]DiscussionCategory, error) diff --git a/pkg/cmd/discussion/client/client_impl.go b/pkg/cmd/discussion/client/client_impl.go index 567cce2af..761913d2a 100644 --- a/pkg/cmd/discussion/client/client_impl.go +++ b/pkg/cmd/discussion/client/client_impl.go @@ -119,7 +119,11 @@ const discussionFields = ` createdAt updatedAt closedAt locked ` -func (c *discussionClient) List(repo ghrepo.Interface, filters ListFilters, limit int) ([]Discussion, int, error) { +func (c *discussionClient) List(repo ghrepo.Interface, filters ListFilters, after string, limit int) (DiscussionListResult, error) { + if limit <= 0 { + return DiscussionListResult{}, fmt.Errorf("limit argument must be positive: %v", limit) + } + type response struct { Repository struct { HasDiscussionsEnabled bool `json:"hasDiscussionsEnabled"` @@ -142,10 +146,24 @@ func (c *discussionClient) List(repo ghrepo.Interface, filters ListFilters, limi orderField := "UPDATED_AT" orderDir := "DESC" if filters.OrderBy != "" { - orderField = strings.ToUpper(filters.OrderBy) + "_AT" + switch filters.OrderBy { + case OrderByCreated: + orderField = "CREATED_AT" + case OrderByUpdated: + orderField = "UPDATED_AT" + default: + return DiscussionListResult{}, fmt.Errorf("unknown order-by field: %q", filters.OrderBy) + } } if filters.Direction != "" { - orderDir = strings.ToUpper(filters.Direction) + switch filters.Direction { + case OrderDirectionAsc: + orderDir = "ASC" + case OrderDirectionDesc: + orderDir = "DESC" + default: + return DiscussionListResult{}, fmt.Errorf("unknown order direction: %q", filters.Direction) + } } variables["orderBy"] = map[string]string{ "field": orderField, @@ -156,11 +174,15 @@ func (c *discussionClient) List(repo ghrepo.Interface, filters ListFilters, limi variables["categoryId"] = filters.CategoryID } - switch strings.ToLower(filters.State) { - case "open": - variables["states"] = []string{"OPEN"} - case "closed": - variables["states"] = []string{"CLOSED"} + if filters.State != nil { + switch *filters.State { + case FilterStateOpen: + variables["states"] = []string{"OPEN"} + case FilterStateClosed: + variables["states"] = []string{"CLOSED"} + default: + return DiscussionListResult{}, fmt.Errorf("unknown state filter: %q; should be one of %q, %q", *filters.State, FilterStateOpen, FilterStateClosed) + } } if filters.Answered != nil { @@ -204,12 +226,20 @@ func (c *discussionClient) List(repo ghrepo.Interface, filters ListFilters, limi } }`, strings.Join(paramParts, ", "), strings.Join(argParts, ", "), discussionFields) + if after != "" { + variables["after"] = after + } + var discussions []Discussion var totalCount int - pageLimit := limit + var nextCursor string + remaining := limit + + // Check hasDiscussionsEnabled on first request only + firstPage := true for { - perPage := pageLimit + perPage := remaining if perPage > 100 { perPage = 100 } @@ -217,29 +247,41 @@ func (c *discussionClient) List(repo ghrepo.Interface, filters ListFilters, limi var data response if err := c.gql.GraphQL(repo.RepoHost(), query, variables, &data); err != nil { - return nil, 0, err + return DiscussionListResult{}, err } - if !data.Repository.HasDiscussionsEnabled { - return nil, 0, fmt.Errorf("the '%s/%s' repository has discussions disabled", repo.RepoOwner(), repo.RepoName()) + if firstPage && !data.Repository.HasDiscussionsEnabled { + return DiscussionListResult{}, fmt.Errorf("the '%s/%s' repository has discussions disabled", repo.RepoOwner(), repo.RepoName()) } + firstPage = false totalCount = data.Repository.Discussions.TotalCount for _, n := range data.Repository.Discussions.Nodes { discussions = append(discussions, mapDiscussion(n)) } - pageLimit -= len(data.Repository.Discussions.Nodes) - if pageLimit <= 0 || !data.Repository.Discussions.PageInfo.HasNextPage { + remaining -= len(data.Repository.Discussions.Nodes) + if remaining <= 0 || !data.Repository.Discussions.PageInfo.HasNextPage { + if data.Repository.Discussions.PageInfo.HasNextPage { + nextCursor = data.Repository.Discussions.PageInfo.EndCursor + } break } variables["after"] = data.Repository.Discussions.PageInfo.EndCursor } - return discussions, totalCount, nil + return DiscussionListResult{ + Discussions: discussions, + TotalCount: totalCount, + NextCursor: nextCursor, + }, nil } -func (c *discussionClient) Search(repo ghrepo.Interface, filters SearchFilters, limit int) ([]Discussion, int, error) { +func (c *discussionClient) Search(repo ghrepo.Interface, filters SearchFilters, after string, limit int) (DiscussionListResult, error) { + if limit <= 0 { + return DiscussionListResult{}, fmt.Errorf("limit argument must be positive: %v", limit) + } + type response struct { Search struct { DiscussionCount int `json:"discussionCount"` @@ -251,43 +293,60 @@ func (c *discussionClient) Search(repo ghrepo.Interface, filters SearchFilters, } `json:"search"` } - searchTerms := []string{fmt.Sprintf("repo:%s/%s", repo.RepoOwner(), repo.RepoName())} + qualifiers := []string{fmt.Sprintf("repo:%s/%s", repo.RepoOwner(), repo.RepoName())} - switch strings.ToLower(filters.State) { - case "open": - searchTerms = append(searchTerms, "state:open") - case "closed": - searchTerms = append(searchTerms, "state:closed") - } - - if filters.Author != "" { - searchTerms = append(searchTerms, fmt.Sprintf("author:%s", filters.Author)) - } - for _, l := range filters.Labels { - searchTerms = append(searchTerms, fmt.Sprintf("label:%q", l)) - } - if filters.Category != "" { - searchTerms = append(searchTerms, fmt.Sprintf("category:%q", filters.Category)) - } - if filters.Answered != nil { - if *filters.Answered { - searchTerms = append(searchTerms, "is:answered") - } else { - searchTerms = append(searchTerms, "is:unanswered") + if filters.State != nil { + switch *filters.State { + case FilterStateOpen: + qualifiers = append(qualifiers, "state:open") + case FilterStateClosed: + qualifiers = append(qualifiers, "state:closed") + default: + return DiscussionListResult{}, fmt.Errorf("unknown state filter: %q; should be one of %q, %q", *filters.State, FilterStateOpen, FilterStateClosed) } } - orderField := "updated" - orderDir := "desc" + if filters.Author != "" { + qualifiers = append(qualifiers, fmt.Sprintf("author:%q", filters.Author)) + } + for _, l := range filters.Labels { + qualifiers = append(qualifiers, fmt.Sprintf("label:%q", l)) + } + if filters.Category != "" { + qualifiers = append(qualifiers, fmt.Sprintf("category:%q", filters.Category)) + } + if filters.Answered != nil { + if *filters.Answered { + qualifiers = append(qualifiers, "is:answered") + } else { + qualifiers = append(qualifiers, "is:unanswered") + } + } + + orderField := OrderByUpdated + orderDir := OrderDirectionDesc if filters.OrderBy != "" { - orderField = strings.ToLower(filters.OrderBy) + switch filters.OrderBy { + case OrderByCreated, OrderByUpdated: + orderField = filters.OrderBy + default: + return DiscussionListResult{}, fmt.Errorf("unknown order-by field: %q", filters.OrderBy) + } } if filters.Direction != "" { - orderDir = strings.ToLower(filters.Direction) + switch filters.Direction { + case OrderDirectionAsc, OrderDirectionDesc: + orderDir = filters.Direction + default: + return DiscussionListResult{}, fmt.Errorf("unknown order direction: %q", filters.Direction) + } } - searchTerms = append(searchTerms, fmt.Sprintf("sort:%s-%s", orderField, orderDir)) + qualifiers = append(qualifiers, fmt.Sprintf("sort:%s-%s", orderField, orderDir)) - searchQuery := strings.Join(searchTerms, " ") + searchQuery := strings.Join(qualifiers, " ") + if filters.Keywords != "" { + searchQuery += " " + filters.Keywords + } query := fmt.Sprintf(`query DiscussionSearch($query: String!, $first: Int!, $after: String) { search(query: $query, type: DISCUSSION, first: $first, after: $after) { @@ -301,12 +360,17 @@ func (c *discussionClient) Search(repo ghrepo.Interface, filters SearchFilters, "query": searchQuery, } + if after != "" { + variables["after"] = after + } + var discussions []Discussion var totalCount int - pageLimit := limit + var nextCursor string + remaining := limit for { - perPage := pageLimit + perPage := remaining if perPage > 100 { perPage = 100 } @@ -314,7 +378,7 @@ func (c *discussionClient) Search(repo ghrepo.Interface, filters SearchFilters, var data response if err := c.gql.GraphQL(repo.RepoHost(), query, variables, &data); err != nil { - return nil, 0, err + return DiscussionListResult{}, err } totalCount = data.Search.DiscussionCount @@ -322,14 +386,21 @@ func (c *discussionClient) Search(repo ghrepo.Interface, filters SearchFilters, discussions = append(discussions, mapDiscussion(n)) } - pageLimit -= len(data.Search.Nodes) - if pageLimit <= 0 || !data.Search.PageInfo.HasNextPage { + remaining -= len(data.Search.Nodes) + if remaining <= 0 || !data.Search.PageInfo.HasNextPage { + if data.Search.PageInfo.HasNextPage { + nextCursor = data.Search.PageInfo.EndCursor + } break } variables["after"] = data.Search.PageInfo.EndCursor } - return discussions, totalCount, nil + return DiscussionListResult{ + Discussions: discussions, + TotalCount: totalCount, + NextCursor: nextCursor, + }, nil } func (c *discussionClient) GetByNumber(_ ghrepo.Interface, _ int) (*Discussion, error) { diff --git a/pkg/cmd/discussion/client/client_mock.go b/pkg/cmd/discussion/client/client_mock.go index 4f8227d5f..589258b4f 100644 --- a/pkg/cmd/discussion/client/client_mock.go +++ b/pkg/cmd/discussion/client/client_mock.go @@ -33,7 +33,7 @@ var _ DiscussionClient = &DiscussionClientMock{} // GetWithCommentsFunc: func(repo ghrepo.Interface, number int, commentLimit int, order string) (*Discussion, error) { // panic("mock out the GetWithComments method") // }, -// ListFunc: func(repo ghrepo.Interface, filters ListFilters, limit int) ([]Discussion, int, error) { +// ListFunc: func(repo ghrepo.Interface, filters ListFilters, after string, limit int) (DiscussionListResult, error) { // panic("mock out the List method") // }, // ListCategoriesFunc: func(repo ghrepo.Interface) ([]DiscussionCategory, error) { @@ -48,7 +48,7 @@ var _ DiscussionClient = &DiscussionClientMock{} // ReopenFunc: func(repo ghrepo.Interface, id string) (*Discussion, error) { // panic("mock out the Reopen method") // }, -// SearchFunc: func(repo ghrepo.Interface, filters SearchFilters, limit int) ([]Discussion, int, error) { +// SearchFunc: func(repo ghrepo.Interface, filters SearchFilters, after string, limit int) (DiscussionListResult, error) { // panic("mock out the Search method") // }, // UnlockFunc: func(repo ghrepo.Interface, id string) error { @@ -83,7 +83,7 @@ type DiscussionClientMock struct { GetWithCommentsFunc func(repo ghrepo.Interface, number int, commentLimit int, order string) (*Discussion, error) // ListFunc mocks the List method. - ListFunc func(repo ghrepo.Interface, filters ListFilters, limit int) ([]Discussion, int, error) + ListFunc func(repo ghrepo.Interface, filters ListFilters, after string, limit int) (DiscussionListResult, error) // ListCategoriesFunc mocks the ListCategories method. ListCategoriesFunc func(repo ghrepo.Interface) ([]DiscussionCategory, error) @@ -98,7 +98,7 @@ type DiscussionClientMock struct { ReopenFunc func(repo ghrepo.Interface, id string) (*Discussion, error) // SearchFunc mocks the Search method. - SearchFunc func(repo ghrepo.Interface, filters SearchFilters, limit int) ([]Discussion, int, error) + SearchFunc func(repo ghrepo.Interface, filters SearchFilters, after string, limit int) (DiscussionListResult, error) // UnlockFunc mocks the Unlock method. UnlockFunc func(repo ghrepo.Interface, id string) error @@ -162,6 +162,8 @@ type DiscussionClientMock struct { Repo ghrepo.Interface // Filters is the filters argument value. Filters ListFilters + // After is the after argument value. + After string // Limit is the limit argument value. Limit int } @@ -199,6 +201,8 @@ type DiscussionClientMock struct { Repo ghrepo.Interface // Filters is the filters argument value. Filters SearchFilters + // After is the after argument value. + After string // Limit is the limit argument value. Limit int } @@ -441,23 +445,25 @@ func (mock *DiscussionClientMock) GetWithCommentsCalls() []struct { } // List calls ListFunc. -func (mock *DiscussionClientMock) List(repo ghrepo.Interface, filters ListFilters, limit int) ([]Discussion, int, error) { +func (mock *DiscussionClientMock) List(repo ghrepo.Interface, filters ListFilters, after string, limit int) (DiscussionListResult, error) { if mock.ListFunc == nil { panic("DiscussionClientMock.ListFunc: method is nil but DiscussionClient.List was just called") } callInfo := struct { Repo ghrepo.Interface Filters ListFilters + After string Limit int }{ Repo: repo, Filters: filters, + After: after, Limit: limit, } mock.lockList.Lock() mock.calls.List = append(mock.calls.List, callInfo) mock.lockList.Unlock() - return mock.ListFunc(repo, filters, limit) + return mock.ListFunc(repo, filters, after, limit) } // ListCalls gets all the calls that were made to List. @@ -467,11 +473,13 @@ func (mock *DiscussionClientMock) List(repo ghrepo.Interface, filters ListFilter func (mock *DiscussionClientMock) ListCalls() []struct { Repo ghrepo.Interface Filters ListFilters + After string Limit int } { var calls []struct { Repo ghrepo.Interface Filters ListFilters + After string Limit int } mock.lockList.RLock() @@ -625,23 +633,25 @@ func (mock *DiscussionClientMock) ReopenCalls() []struct { } // Search calls SearchFunc. -func (mock *DiscussionClientMock) Search(repo ghrepo.Interface, filters SearchFilters, limit int) ([]Discussion, int, error) { +func (mock *DiscussionClientMock) Search(repo ghrepo.Interface, filters SearchFilters, after string, limit int) (DiscussionListResult, error) { if mock.SearchFunc == nil { panic("DiscussionClientMock.SearchFunc: method is nil but DiscussionClient.Search was just called") } callInfo := struct { Repo ghrepo.Interface Filters SearchFilters + After string Limit int }{ Repo: repo, Filters: filters, + After: after, Limit: limit, } mock.lockSearch.Lock() mock.calls.Search = append(mock.calls.Search, callInfo) mock.lockSearch.Unlock() - return mock.SearchFunc(repo, filters, limit) + return mock.SearchFunc(repo, filters, after, limit) } // SearchCalls gets all the calls that were made to Search. @@ -651,11 +661,13 @@ func (mock *DiscussionClientMock) Search(repo ghrepo.Interface, filters SearchFi func (mock *DiscussionClientMock) SearchCalls() []struct { Repo ghrepo.Interface Filters SearchFilters + After string Limit int } { var calls []struct { Repo ghrepo.Interface Filters SearchFilters + After string Limit int } mock.lockSearch.RLock() diff --git a/pkg/cmd/discussion/client/types.go b/pkg/cmd/discussion/client/types.go index 9870416ad..15cd45936 100644 --- a/pkg/cmd/discussion/client/types.go +++ b/pkg/cmd/discussion/client/types.go @@ -225,10 +225,37 @@ const ( CloseReasonDuplicate CloseReason = "DUPLICATE" ) +// Domain-level filter constants for state. +const ( + FilterStateOpen = "open" + FilterStateClosed = "closed" +) + +// Domain-level constants for order-by field. +const ( + OrderByCreated = "created" + OrderByUpdated = "updated" +) + +// Domain-level constants for order direction. +const ( + OrderDirectionAsc = "asc" + OrderDirectionDesc = "desc" +) + +// DiscussionListResult holds the result of a List or Search call, +// including the discussions, total count, and pagination cursor. +type DiscussionListResult struct { + Discussions []Discussion + TotalCount int + NextCursor string +} + // ListFilters holds parameters for the repository.discussions query. // CategoryID must be resolved by the caller before passing to List. +// A nil State indicates no state filtering (all states). type ListFilters struct { - State string + State *string CategoryID string Answered *bool OrderBy string @@ -237,12 +264,14 @@ type ListFilters struct { // SearchFilters holds parameters for the search query used when // author or label filtering is required. +// A nil State indicates no state filtering (all states). type SearchFilters struct { Author string Labels []string - State string + State *string Category string Answered *bool + Keywords string OrderBy string Direction string } diff --git a/pkg/cmd/discussion/discussion.go b/pkg/cmd/discussion/discussion.go index bd724fe69..a9bf9b981 100644 --- a/pkg/cmd/discussion/discussion.go +++ b/pkg/cmd/discussion/discussion.go @@ -11,8 +11,10 @@ import ( func NewCmdDiscussion(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ Use: "discussion ", - Short: "Manage discussions", - Long: "Work with GitHub Discussions.", + Short: "Work with GitHub Discussions (preview)", + Long: heredoc.Doc(` + Working with discussions in the GitHub CLI is in preview and subject to change without notice. + `), Example: heredoc.Doc(` $ gh discussion list $ gh discussion create --category "General" --title "Hello" diff --git a/pkg/cmd/discussion/list/list.go b/pkg/cmd/discussion/list/list.go index f82cc9806..4a1c5b509 100644 --- a/pkg/cmd/discussion/list/list.go +++ b/pkg/cmd/discussion/list/list.go @@ -18,6 +18,8 @@ import ( "github.com/spf13/cobra" ) +const defaultLimit = 30 + // ListOptions holds the configuration for the discussion list command. type ListOptions struct { IO *iostreams.IOStreams @@ -31,7 +33,10 @@ type ListOptions struct { State string Limit int Answered *bool + Sort string Order string + Search string + After string WebMode bool Exporter cmdutil.Exporter @@ -47,8 +52,8 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman } cmd := &cobra.Command{ - Use: "list", - Short: "List discussions in a repository", + Use: "list [flags]", + Short: "List discussions in a repository (preview)", Long: heredoc.Doc(` List discussions in a GitHub repository. By default, only open discussions are shown. @@ -58,13 +63,19 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman $ gh discussion list # List discussions with a specific category - $ gh discussion list --category "General" + $ gh discussion list --category General # List closed discussions by author $ gh discussion list --state closed --author monalisa + # List all discussions (closed or open) by label + $ gh discussion list --state all --label bug,enhancement + # List answered discussions as JSON $ gh discussion list --answered --json number,title,url + + # List unanswered discussions as JSON + $ gh discussion list --answered=false --json number,title,url `), Aliases: []string{"ls"}, Args: cmdutil.NoArgsQuoteReminder, @@ -87,15 +98,33 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman cmd.Flags().StringVarP(&opts.Category, "category", "c", "", "Filter by category name or slug") cmd.Flags().StringSliceVarP(&opts.Labels, "label", "l", nil, "Filter by label") cmdutil.StringEnumFlag(cmd, &opts.State, "state", "s", "open", []string{"open", "closed", "all"}, "Filter by state") - cmd.Flags().IntVarP(&opts.Limit, "limit", "L", 30, "Maximum number of discussions to fetch") + cmd.Flags().IntVarP(&opts.Limit, "limit", "L", defaultLimit, fmt.Sprintf("Maximum number of discussions to fetch (default %d)", defaultLimit)) cmdutil.NilBoolFlag(cmd, &opts.Answered, "answered", "", "Filter by answered state") - cmdutil.StringEnumFlag(cmd, &opts.Order, "order", "", "updated", []string{"created", "updated"}, "Order by field") + cmdutil.StringEnumFlag(cmd, &opts.Sort, "sort", "", "updated", []string{"created", "updated"}, "Sort by field") + cmdutil.StringEnumFlag(cmd, &opts.Order, "order", "", "desc", []string{"asc", "desc"}, "Order of results") + cmd.Flags().StringVarP(&opts.Search, "search", "S", "", "Search discussions with `query`") + cmd.Flags().StringVar(&opts.After, "after", "", "Cursor for the next page of results") cmd.Flags().BoolVarP(&opts.WebMode, "web", "w", false, "List discussions in the web browser") cmdutil.AddJSONFlags(cmd, &opts.Exporter, shared.DiscussionFields) return cmd } +// toFilterState maps CLI state strings to domain-level filter state pointers. +// "all" maps to nil (no state filter). +func toFilterState(v string) *string { + switch v { + case "open": + s := client.FilterStateOpen + return &s + case "closed": + s := client.FilterStateClosed + return &s + default: + return nil + } +} + func listRun(opts *ListOptions) error { repo, err := opts.BaseRepo() if err != nil { @@ -118,7 +147,7 @@ func listRun(opts *ListOptions) error { if err != nil { return err } - cat, err := matchCategory(opts.Category, categories) + cat, err := shared.MatchCategory(opts.Category, categories) if err != nil { return err } @@ -126,28 +155,32 @@ func listRun(opts *ListOptions) error { categorySlug = cat.Slug } - var discussions []client.Discussion - var totalCount int + state := toFilterState(opts.State) - useSearch := opts.Author != "" || len(opts.Labels) > 0 + var result client.DiscussionListResult + + useSearch := opts.Author != "" || len(opts.Labels) > 0 || opts.Search != "" if useSearch { filters := client.SearchFilters{ - Author: opts.Author, - Labels: opts.Labels, - State: opts.State, - Category: categorySlug, - Answered: opts.Answered, - OrderBy: opts.Order, + Author: opts.Author, + Labels: opts.Labels, + State: state, + Category: categorySlug, + Answered: opts.Answered, + Keywords: opts.Search, + OrderBy: opts.Sort, + Direction: opts.Order, } - discussions, totalCount, err = dc.Search(repo, filters, opts.Limit) + result, err = dc.Search(repo, filters, opts.After, opts.Limit) } else { filters := client.ListFilters{ - State: opts.State, + State: state, CategoryID: categoryID, Answered: opts.Answered, - OrderBy: opts.Order, + OrderBy: opts.Sort, + Direction: opts.Order, } - discussions, totalCount, err = dc.List(repo, filters, opts.Limit) + result, err = dc.List(repo, filters, opts.After, opts.Limit) } if err != nil { return err @@ -155,13 +188,14 @@ func listRun(opts *ListOptions) error { if opts.Exporter != nil { envelope := map[string]interface{}{ - "totalCount": totalCount, - "discussions": discussions, + "totalCount": result.TotalCount, + "discussions": result.Discussions, + "next": result.NextCursor, } return opts.Exporter.Write(opts.IO, envelope) } - if len(discussions) == 0 { + if len(result.Discussions) == 0 { return noResults(repo, opts.State) } @@ -173,11 +207,11 @@ func listRun(opts *ListOptions) error { isTerminal := opts.IO.IsStdoutTTY() if isTerminal { - title := listHeader(ghrepo.FullName(repo), len(discussions), totalCount, opts.State) + title := listHeader(ghrepo.FullName(repo), len(result.Discussions), result.TotalCount, opts.State) fmt.Fprintf(opts.IO.Out, "\n%s\n\n", title) } - printDiscussions(opts, discussions, totalCount) + printDiscussions(opts, result.Discussions, result.TotalCount) return nil } @@ -215,25 +249,6 @@ func openInBrowser(opts *ListOptions, repo ghrepo.Interface) error { return opts.Browser.Browse(discussionsURL) } -func matchCategory(input string, categories []client.DiscussionCategory) (*client.DiscussionCategory, error) { - for i := range categories { - if strings.EqualFold(categories[i].Slug, input) { - return &categories[i], nil - } - } - for i := range categories { - if strings.EqualFold(categories[i].Name, input) { - return &categories[i], nil - } - } - - var available strings.Builder - for _, c := range categories { - fmt.Fprintf(&available, " %s (%s)\n", c.Slug, c.Name) - } - return nil, fmt.Errorf("category not found: %s\n\nAvailable categories:\n%s", input, available.String()) -} - func noResults(repo ghrepo.Interface, state string) error { stateQualifier := "" switch state { diff --git a/pkg/cmd/discussion/list/list_test.go b/pkg/cmd/discussion/list/list_test.go index f4e6f57ab..9404ac806 100644 --- a/pkg/cmd/discussion/list/list_test.go +++ b/pkg/cmd/discussion/list/list_test.go @@ -55,6 +55,13 @@ func sampleDiscussions() []client.Discussion { } } +func sampleResult() client.DiscussionListResult { + return client.DiscussionListResult{ + Discussions: sampleDiscussions(), + TotalCount: 2, + } +} + func sampleCategories() []client.DiscussionCategory { return []client.DiscussionCategory{ {ID: "CAT1", Name: "General", Slug: "general", IsAnswerable: true}, @@ -69,8 +76,8 @@ func TestListRun_tty(t *testing.T) { ios.SetStderrTTY(true) mockClient := &client.DiscussionClientMock{ - ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, limit int) ([]client.Discussion, int, error) { - return sampleDiscussions(), 2, nil + ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, after string, limit int) (client.DiscussionListResult, error) { + return sampleResult(), nil }, } @@ -80,6 +87,8 @@ func TestListRun_tty(t *testing.T) { Client: func() (client.DiscussionClient, error) { return mockClient, nil }, State: "open", Limit: 30, + Sort: "updated", + Order: "desc", Now: fixedTime, } @@ -102,8 +111,8 @@ func TestListRun_nontty(t *testing.T) { ios.SetStdoutTTY(false) mockClient := &client.DiscussionClientMock{ - ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, limit int) ([]client.Discussion, int, error) { - return sampleDiscussions(), 2, nil + ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, after string, limit int) (client.DiscussionListResult, error) { + return sampleResult(), nil }, } @@ -113,6 +122,8 @@ func TestListRun_nontty(t *testing.T) { Client: func() (client.DiscussionClient, error) { return mockClient, nil }, State: "open", Limit: 30, + Sort: "updated", + Order: "desc", Now: fixedTime, } @@ -120,7 +131,6 @@ func TestListRun_nontty(t *testing.T) { require.NoError(t, err) out := stdout.String() - // Non-TTY output should not contain # prefix or header assert.NotContains(t, out, "Showing") assert.Contains(t, out, "42") assert.Contains(t, out, "OPEN") @@ -131,8 +141,12 @@ func TestListRun_json(t *testing.T) { ios, _, stdout, _ := iostreams.Test() mockClient := &client.DiscussionClientMock{ - ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, limit int) ([]client.Discussion, int, error) { - return sampleDiscussions(), 2, nil + ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, after string, limit int) (client.DiscussionListResult, error) { + return client.DiscussionListResult{ + Discussions: sampleDiscussions(), + TotalCount: 2, + NextCursor: "CURSOR123", + }, nil }, } @@ -145,6 +159,8 @@ func TestListRun_json(t *testing.T) { Client: func() (client.DiscussionClient, error) { return mockClient, nil }, State: "open", Limit: 30, + Sort: "updated", + Order: "desc", Exporter: exporter, Now: fixedTime, } @@ -155,6 +171,7 @@ func TestListRun_json(t *testing.T) { out := stdout.String() assert.Contains(t, out, `"totalCount"`) assert.Contains(t, out, `"discussions"`) + assert.Contains(t, out, `"next"`) } func TestListRun_web(t *testing.T) { @@ -184,8 +201,8 @@ func TestListRun_noResults(t *testing.T) { ios.SetStdoutTTY(true) mockClient := &client.DiscussionClientMock{ - ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, limit int) ([]client.Discussion, int, error) { - return []client.Discussion{}, 0, nil + ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, after string, limit int) (client.DiscussionListResult, error) { + return client.DiscussionListResult{}, nil }, } @@ -195,6 +212,8 @@ func TestListRun_noResults(t *testing.T) { Client: func() (client.DiscussionClient, error) { return mockClient, nil }, State: "open", Limit: 30, + Sort: "updated", + Order: "desc", Now: fixedTime, } @@ -212,9 +231,12 @@ func TestListRun_categoryFilter(t *testing.T) { ListCategoriesFunc: func(repo ghrepo.Interface) ([]client.DiscussionCategory, error) { return sampleCategories(), nil }, - ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, limit int) ([]client.Discussion, int, error) { + ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, after string, limit int) (client.DiscussionListResult, error) { assert.Equal(t, "CAT1", filters.CategoryID) - return sampleDiscussions()[:1], 1, nil + return client.DiscussionListResult{ + Discussions: sampleDiscussions()[:1], + TotalCount: 1, + }, nil }, } @@ -225,6 +247,8 @@ func TestListRun_categoryFilter(t *testing.T) { Category: "general", State: "open", Limit: 30, + Sort: "updated", + Order: "desc", Now: fixedTime, } @@ -249,13 +273,14 @@ func TestListRun_categoryNotFound(t *testing.T) { Category: "nonexistent", State: "open", Limit: 30, + Sort: "updated", + Order: "desc", Now: fixedTime, } err := listRun(opts) require.Error(t, err) - assert.Contains(t, err.Error(), "category not found: nonexistent") - assert.Contains(t, err.Error(), "general (General)") + assert.Contains(t, err.Error(), `unknown category: "nonexistent"`) } func TestListRun_authorFilter(t *testing.T) { @@ -263,9 +288,12 @@ func TestListRun_authorFilter(t *testing.T) { ios.SetStdoutTTY(true) mockClient := &client.DiscussionClientMock{ - SearchFunc: func(repo ghrepo.Interface, filters client.SearchFilters, limit int) ([]client.Discussion, int, error) { + SearchFunc: func(repo ghrepo.Interface, filters client.SearchFilters, after string, limit int) (client.DiscussionListResult, error) { assert.Equal(t, "monalisa", filters.Author) - return sampleDiscussions()[:1], 1, nil + return client.DiscussionListResult{ + Discussions: sampleDiscussions()[:1], + TotalCount: 1, + }, nil }, } @@ -276,6 +304,8 @@ func TestListRun_authorFilter(t *testing.T) { Author: "monalisa", State: "open", Limit: 30, + Sort: "updated", + Order: "desc", Now: fixedTime, } @@ -289,9 +319,12 @@ func TestListRun_labelFilter(t *testing.T) { ios.SetStdoutTTY(true) mockClient := &client.DiscussionClientMock{ - SearchFunc: func(repo ghrepo.Interface, filters client.SearchFilters, limit int) ([]client.Discussion, int, error) { + SearchFunc: func(repo ghrepo.Interface, filters client.SearchFilters, after string, limit int) (client.DiscussionListResult, error) { assert.Equal(t, []string{"bug", "docs"}, filters.Labels) - return sampleDiscussions()[:1], 1, nil + return client.DiscussionListResult{ + Discussions: sampleDiscussions()[:1], + TotalCount: 1, + }, nil }, } @@ -302,6 +335,8 @@ func TestListRun_labelFilter(t *testing.T) { Labels: []string{"bug", "docs"}, State: "open", Limit: 30, + Sort: "updated", + Order: "desc", Now: fixedTime, } @@ -310,6 +345,64 @@ func TestListRun_labelFilter(t *testing.T) { assert.Contains(t, stdout.String(), "Bug report discussion") } +func TestListRun_searchFilter(t *testing.T) { + ios, _, stdout, _ := iostreams.Test() + ios.SetStdoutTTY(true) + + mockClient := &client.DiscussionClientMock{ + SearchFunc: func(repo ghrepo.Interface, filters client.SearchFilters, after string, limit int) (client.DiscussionListResult, error) { + assert.Equal(t, "some keywords", filters.Keywords) + return client.DiscussionListResult{ + Discussions: sampleDiscussions()[:1], + TotalCount: 1, + }, nil + }, + } + + opts := &ListOptions{ + IO: ios, + BaseRepo: func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, + Client: func() (client.DiscussionClient, error) { return mockClient, nil }, + Search: "some keywords", + State: "open", + Limit: 30, + Sort: "updated", + Order: "desc", + Now: fixedTime, + } + + err := listRun(opts) + require.NoError(t, err) + assert.Contains(t, stdout.String(), "Bug report discussion") +} + +func TestListRun_afterCursor(t *testing.T) { + ios, _, _, _ := iostreams.Test() + ios.SetStdoutTTY(true) + + mockClient := &client.DiscussionClientMock{ + ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, after string, limit int) (client.DiscussionListResult, error) { + assert.Equal(t, "CURSOR_ABC", after) + return sampleResult(), nil + }, + } + + opts := &ListOptions{ + IO: ios, + BaseRepo: func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, + Client: func() (client.DiscussionClient, error) { return mockClient, nil }, + State: "open", + Limit: 30, + Sort: "updated", + Order: "desc", + After: "CURSOR_ABC", + Now: fixedTime, + } + + err := listRun(opts) + require.NoError(t, err) +} + func TestNewCmdList(t *testing.T) { tests := []struct { name string @@ -349,15 +442,41 @@ func TestNewCmdList(t *testing.T) { name: "web flag", args: "--web", }, + { + name: "sort flag", + args: "--sort created", + }, { name: "order flag", - args: "--order created", + args: "--order asc", + }, + { + name: "sort and order flags", + args: "--sort created --order asc", + }, + { + name: "search flag", + args: "--search \"some query\"", + }, + { + name: "after flag", + args: "--after CURSOR123", }, { name: "invalid state", args: "--state invalid", wantsErr: true, }, + { + name: "invalid sort", + args: "--sort invalid", + wantsErr: true, + }, + { + name: "invalid order", + args: "--order invalid", + wantsErr: true, + }, } for _, tt := range tests { @@ -365,8 +484,8 @@ func TestNewCmdList(t *testing.T) { ios, _, _, _ := iostreams.Test() f := &cmdutil.Factory{ IOStreams: ios, - Browser: &browser.Stub{}, - BaseRepo: func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, + Browser: &browser.Stub{}, + BaseRepo: func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, } var gotOpts *ListOptions @@ -395,6 +514,30 @@ func TestNewCmdList(t *testing.T) { } } +func TestToFilterState(t *testing.T) { + tests := []struct { + input string + want *string + }{ + {"open", strPtr(client.FilterStateOpen)}, + {"closed", strPtr(client.FilterStateClosed)}, + {"all", nil}, + } + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + got := toFilterState(tt.input) + if tt.want == nil { + assert.Nil(t, got) + } else { + require.NotNil(t, got) + assert.Equal(t, *tt.want, *got) + } + }) + } +} + +func strPtr(s string) *string { return &s } + func splitArgs(s string) []string { var args []string for _, part := range splitRespectingQuotes(s) { @@ -441,8 +584,11 @@ func TestListRun_closedState(t *testing.T) { } mockClient := &client.DiscussionClientMock{ - ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, limit int) ([]client.Discussion, int, error) { - return closed, 1, nil + ListFunc: func(repo ghrepo.Interface, filters client.ListFilters, after string, limit int) (client.DiscussionListResult, error) { + return client.DiscussionListResult{ + Discussions: closed, + TotalCount: 1, + }, nil }, } @@ -452,6 +598,8 @@ func TestListRun_closedState(t *testing.T) { Client: func() (client.DiscussionClient, error) { return mockClient, nil }, State: "closed", Limit: 30, + Sort: "updated", + Order: "desc", Now: fixedTime, } @@ -461,6 +609,5 @@ func TestListRun_closedState(t *testing.T) { out := stdout.String() assert.Contains(t, out, "closed discussions") assert.Contains(t, out, "Old discussion") - // Verify the # prefix is present (TTY mode) assert.Contains(t, out, "#10") } diff --git a/pkg/cmd/discussion/shared/categories.go b/pkg/cmd/discussion/shared/categories.go new file mode 100644 index 000000000..a7893f229 --- /dev/null +++ b/pkg/cmd/discussion/shared/categories.go @@ -0,0 +1,32 @@ +package shared + +import ( + "fmt" + "slices" + "strings" + + "github.com/cli/cli/v2/pkg/cmd/discussion/client" +) + +// MatchCategory finds a category by name or slug (case-insensitive). +// It prefers an exact slug match over a name match, so users are +// encouraged to use slugs for unambiguous lookups. +func MatchCategory(input string, categories []client.DiscussionCategory) (*client.DiscussionCategory, error) { + for i := range categories { + if strings.EqualFold(categories[i].Slug, input) { + return &categories[i], nil + } + } + for i := range categories { + if strings.EqualFold(categories[i].Name, input) { + return &categories[i], nil + } + } + + slugs := make([]string, len(categories)) + for i, c := range categories { + slugs[i] = c.Slug + } + slices.Sort(slugs) + return nil, fmt.Errorf("unknown category: %q; must be one of %v", input, slugs) +}