diff --git a/internal/app/issue_list.go b/internal/app/issue_list.go index 266d82f..d65adee 100644 --- a/internal/app/issue_list.go +++ b/internal/app/issue_list.go @@ -3,21 +3,84 @@ package app import ( "context" "fmt" + "strings" "github.com/alyraffauf/tg/tangled" ) -// ListIssues lists every issue in the target repository. -func (s *Service) ListIssues(ctx context.Context, t Target) ([]Item, error) { +// ListOptions filters and limits issue and pull-request listings. +type ListOptions struct { + Author string + State string + Limit int64 + Order string +} + +// ListIssues lists issues in the target repository. +func (s *Service) ListIssues(ctx context.Context, t Target, options ListOptions) ([]Item, error) { repoDid, err := s.repoDID(ctx, t) if err != nil { return nil, err } - issues, err := s.appview.ListIssues(ctx, repoDid, tangled.ListOpts{ - Limit: defaultListLimit, - }) + listOptions, err := s.issueListOptions(ctx, options) + if err != nil { + return nil, err + } + issues, err := s.appview.ListIssues(ctx, repoDid, listOptions) if err != nil { return nil, fmt.Errorf("list issues for %q: %w", t.Repo, err) } return s.buildItems(ctx, issues.Items, decodeIssue), nil } + +func (s *Service) issueListOptions(ctx context.Context, options ListOptions) (tangled.ListOpts, error) { + return s.appviewListOptions(ctx, options, "issue", "open", "closed") +} + +func (s *Service) pullListOptions(ctx context.Context, options ListOptions) (tangled.ListOpts, error) { + return s.appviewListOptions(ctx, options, "pull request", "open", "closed", "merged") +} + +func (s *Service) appviewListOptions(ctx context.Context, options ListOptions, resource string, states ...string) (tangled.ListOpts, error) { + if options.Limit < 0 { + return tangled.ListOpts{}, fmt.Errorf("%s list limit must be positive", resource) + } + if options.State != "" && !isAllowedValue(options.State, states) { + return tangled.ListOpts{}, fmt.Errorf("invalid %s state %q", resource, options.State) + } + if options.Order != "" && !isAllowedValue(options.Order, []string{"asc", "desc"}) { + return tangled.ListOpts{}, fmt.Errorf("invalid list order %q", options.Order) + } + + author, err := s.resolveAuthorFilter(ctx, options.Author) + if err != nil { + return tangled.ListOpts{}, err + } + return tangled.ListOpts{ + Author: author, + State: options.State, + Limit: defaultListLimit, + MaxItems: options.Limit, + Order: options.Order, + }, nil +} + +func (s *Service) resolveAuthorFilter(ctx context.Context, author string) (string, error) { + if author == "" || strings.HasPrefix(author, "did:") { + return author, nil + } + identity, err := s.resolver.ResolveHandle(ctx, author) + if err != nil { + return "", fmt.Errorf("resolve author %q: %w", author, err) + } + return identity.DID.String(), nil +} + +func isAllowedValue(value string, allowed []string) bool { + for _, candidate := range allowed { + if value == candidate { + return true + } + } + return false +} diff --git a/internal/app/pull_list.go b/internal/app/pull_list.go index b651844..4d2d6af 100644 --- a/internal/app/pull_list.go +++ b/internal/app/pull_list.go @@ -3,19 +3,19 @@ package app import ( "context" "fmt" - - "github.com/alyraffauf/tg/tangled" ) -// ListPulls lists every pull request in the target repository. -func (s *Service) ListPulls(ctx context.Context, t Target) ([]Item, error) { +// ListPulls lists pull requests in the target repository. +func (s *Service) ListPulls(ctx context.Context, t Target, options ListOptions) ([]Item, error) { repoDid, err := s.repoDID(ctx, t) if err != nil { return nil, err } - pulls, err := s.appview.ListPulls(ctx, repoDid, tangled.ListOpts{ - Limit: defaultListLimit, - }) + listOptions, err := s.pullListOptions(ctx, options) + if err != nil { + return nil, err + } + pulls, err := s.appview.ListPulls(ctx, repoDid, listOptions) if err != nil { return nil, fmt.Errorf("list PRs for %q: %w", t.Repo, err) } diff --git a/internal/app/service_test.go b/internal/app/service_test.go index 8f16870..65f99ab 100644 --- a/internal/app/service_test.go +++ b/internal/app/service_test.go @@ -10,6 +10,7 @@ import ( "io" "net/http" "net/http/httptest" + "reflect" "strings" "sync" "testing" @@ -230,6 +231,38 @@ func (r testResolver) ResolvePDS(context.Context, string) (string, error) { return "https://pds.example", nil } +func TestListOptions(t *testing.T) { + service := testService(nil, nil, nil) + + issueOptions, err := service.issueListOptions(context.Background(), ListOptions{ + Author: "alice.test", + State: "open", + Limit: 20, + Order: "asc", + }) + if err != nil { + t.Fatalf("issueListOptions() error = %v", err) + } + want := tangled.ListOpts{Author: "did:plc:owner", State: "open", Limit: defaultListLimit, MaxItems: 20, Order: "asc"} + if !reflect.DeepEqual(issueOptions, want) { + t.Fatalf("issueListOptions() = %#v, want %#v", issueOptions, want) + } + + pullOptions, err := service.pullListOptions(context.Background(), ListOptions{Author: "did:plc:alice", State: "merged"}) + if err != nil { + t.Fatalf("pullListOptions() error = %v", err) + } + if pullOptions.Author != "did:plc:alice" || pullOptions.State != "merged" { + t.Fatalf("pullListOptions() = %#v", pullOptions) + } + + for _, options := range []ListOptions{{State: "merged"}, {Order: "newest"}, {Limit: -1}} { + if _, err := service.issueListOptions(context.Background(), options); err == nil { + t.Fatalf("issueListOptions(%#v) succeeded, want validation error", options) + } + } +} + type testSessions struct { pds pdsClient api *atclient.APIClient diff --git a/internal/cli/issue_list.go b/internal/cli/issue_list.go index 1767ef0..8c6ae20 100644 --- a/internal/cli/issue_list.go +++ b/internal/cli/issue_list.go @@ -1,12 +1,41 @@ package cli import ( + "fmt" + "github.com/alyraffauf/tg/internal/app" "github.com/spf13/cobra" ) +type listFlags struct { + author string + state string + limit int64 + order string +} + +func (flags *listFlags) add(command *cobra.Command) { + command.Flags().StringVar(&flags.author, "author", "", "Filter by author handle or DID") + command.Flags().StringVar(&flags.state, "state", "", "Filter by state") + command.Flags().Int64Var(&flags.limit, "limit", 0, "Maximum number of results") + command.Flags().StringVar(&flags.order, "order", "", "Sort order: asc or desc") +} + +func (flags listFlags) options(command *cobra.Command) (app.ListOptions, error) { + if command.Flags().Changed("limit") && flags.limit <= 0 { + return app.ListOptions{}, fmt.Errorf("list limit must be positive") + } + return app.ListOptions{ + Author: flags.author, + State: flags.state, + Limit: flags.limit, + Order: flags.order, + }, nil +} + func newIssueListCommand(service *app.Service) *cobra.Command { - return &cobra.Command{ + var flags listFlags + command := &cobra.Command{ Use: "list [handle/repo]", Short: "List issues for a Tangled repository", Long: `List issues for a Tangled repository. @@ -15,12 +44,16 @@ If no argument is given, the command detects the repository from the "origin" remote URL of the git repository in the current directory.`, Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + options, err := flags.options(cmd) + if err != nil { + return err + } ctx := cmd.Context() target, err := resolveTarget(ctx, args, service) if err != nil { return err } - items, err := service.ListIssues(ctx, target) + items, err := service.ListIssues(ctx, target, options) if err != nil { return err } @@ -29,4 +62,6 @@ If no argument is given, the command detects the repository from the }) }, } + flags.add(command) + return command } diff --git a/internal/cli/pr_list.go b/internal/cli/pr_list.go index c4c9c77..375bb3a 100644 --- a/internal/cli/pr_list.go +++ b/internal/cli/pr_list.go @@ -6,7 +6,8 @@ import ( ) func newPRListCommand(service *app.Service) *cobra.Command { - return &cobra.Command{ + var flags listFlags + command := &cobra.Command{ Use: "list [handle/repo]", Short: "List pull requests for a Tangled repository", Long: `List pull requests for a Tangled repository. @@ -15,12 +16,16 @@ If no argument is given, the command detects the repository from the "origin" remote URL of the git repository in the current directory.`, Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + options, err := flags.options(cmd) + if err != nil { + return err + } ctx := cmd.Context() target, err := resolveTarget(ctx, args, service) if err != nil { return err } - items, err := service.ListPulls(ctx, target) + items, err := service.ListPulls(ctx, target, options) if err != nil { return err } @@ -29,4 +34,6 @@ If no argument is given, the command detects the repository from the }) }, } + flags.add(command) + return command } diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index 91bd058..1901fe3 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -32,6 +32,31 @@ func TestNewRootCreatesIndependentCommandState(t *testing.T) { } } +func TestListCommandsSupportFilters(t *testing.T) { + root := NewRoot(&app.Service{}) + + for _, path := range [][]string{{"issue", "list"}, {"pr", "list"}} { + command, _, err := root.Find(path) + if err != nil { + t.Fatalf("find %s: %v", strings.Join(path, " "), err) + } + for _, name := range []string{"author", "state", "limit", "order"} { + if command.Flags().Lookup(name) == nil { + t.Fatalf("%s is missing --%s", strings.Join(path, " "), name) + } + } + } + + command := newIssueListCommand(&app.Service{}) + if err := command.Flags().Set("limit", "0"); err != nil { + t.Fatalf("set limit: %v", err) + } + flags := listFlags{limit: 0} + if _, err := flags.options(command); err == nil { + t.Fatal("issue list accepted an explicit zero limit") + } +} + func TestVersionFlag(t *testing.T) { var output bytes.Buffer err := ExecuteWith([]string{"--version"}, nil, &output, &bytes.Buffer{}) diff --git a/tangled/list.go b/tangled/list.go index d32d11a..10fdd12 100644 --- a/tangled/list.go +++ b/tangled/list.go @@ -28,17 +28,22 @@ type List struct { // ListOpts are the query parameters shared by ListIssues and ListPulls. type ListOpts struct { - Author string // only items by this DID - State string // "open" or "closed" - Limit int64 // 1-1000, default 50 - Order string // "asc" or "desc" + Author string // only items by this DID + State string // "open" or "closed" + Limit int64 // page size: 1-1000, default 50 + MaxItems int64 // maximum results to return; zero returns every item + Order string // "asc" or "desc" } func (o ListOpts) limit() int64 { + limit := int64(50) if o.Limit > 0 { - return o.Limit + limit = o.Limit } - return 50 + if o.MaxItems > 0 && o.MaxItems < limit { + return o.MaxItems + } + return limit } // params builds query parameters for a paginated issue or pull request list. @@ -62,7 +67,7 @@ func (o ListOpts) params(subject, cursor string) map[string]any { // fetchAllPages calls fetch for successive pages, advancing the cursor it // returns, until a page reports no further cursor. It returns every item // across all pages combined. -func fetchAllPages[T any](ctx context.Context, fetch func(ctx context.Context, cursor string) (items []T, nextCursor *string, err error)) ([]T, error) { +func fetchAllPages[T any](ctx context.Context, maximumItems int64, fetch func(ctx context.Context, cursor string) (items []T, nextCursor *string, err error)) ([]T, error) { var all []T cursor := "" @@ -71,8 +76,15 @@ func fetchAllPages[T any](ctx context.Context, fetch func(ctx context.Context, c if err != nil { return nil, err } + if maximumItems > 0 && int64(len(items)) > maximumItems-int64(len(all)) { + items = items[:maximumItems-int64(len(all))] + } all = append(all, items...) + if maximumItems > 0 && int64(len(all)) == maximumItems { + return all, nil + } + if nextCursor == nil || *nextCursor == "" { return all, nil } diff --git a/tangled/list_test.go b/tangled/list_test.go index 11eca60..a0fb6f0 100644 --- a/tangled/list_test.go +++ b/tangled/list_test.go @@ -44,6 +44,15 @@ func TestListOptsParams(t *testing.T) { "limit": int64(50), }, }, + { + name: "maximum results reduces page size", + opts: ListOpts{Limit: 100, MaxItems: 20}, + subject: "did:plc:repo", + want: map[string]any{ + "subject": "did:plc:repo", + "limit": int64(20), + }, + }, } for _, tt := range tests { @@ -60,7 +69,7 @@ func TestFetchAllPages(t *testing.T) { t.Run("single page", func(t *testing.T) { calls := 0 - items, err := fetchAllPages(context.Background(), func(_ context.Context, cursor string) ([]string, *string, error) { + items, err := fetchAllPages(context.Background(), 0, func(_ context.Context, cursor string) ([]string, *string, error) { calls++ if cursor != "" { t.Errorf("first fetch got cursor %q, want empty", cursor) @@ -87,7 +96,7 @@ func TestFetchAllPages(t *testing.T) { next := map[string]*string{"": strptr("cursor1"), "cursor1": strptr("cursor2"), "cursor2": nil} var gotCursors []string - items, err := fetchAllPages(context.Background(), func(_ context.Context, cursor string) ([]string, *string, error) { + items, err := fetchAllPages(context.Background(), 0, func(_ context.Context, cursor string) ([]string, *string, error) { gotCursors = append(gotCursors, cursor) pageItems, ok := pages[cursor] if !ok { @@ -108,7 +117,7 @@ func TestFetchAllPages(t *testing.T) { }) t.Run("empty first page", func(t *testing.T) { - items, err := fetchAllPages(context.Background(), func(_ context.Context, _ string) ([]string, *string, error) { + items, err := fetchAllPages(context.Background(), 0, func(_ context.Context, _ string) ([]string, *string, error) { return nil, nil, nil }) if err != nil { @@ -121,7 +130,7 @@ func TestFetchAllPages(t *testing.T) { t.Run("error propagates", func(t *testing.T) { errFetch := errors.New("fetch failed") - items, err := fetchAllPages(context.Background(), func(_ context.Context, cursor string) ([]string, *string, error) { + items, err := fetchAllPages(context.Background(), 0, func(_ context.Context, cursor string) ([]string, *string, error) { if cursor != "" { return nil, nil, errFetch } @@ -137,7 +146,7 @@ func TestFetchAllPages(t *testing.T) { t.Run("runaway cursor", func(t *testing.T) { calls := 0 - _, err := fetchAllPages(context.Background(), func(_ context.Context, _ string) ([]string, *string, error) { + _, err := fetchAllPages(context.Background(), 0, func(_ context.Context, _ string) ([]string, *string, error) { calls++ return []string{"a"}, strptr("next"), nil }) @@ -148,4 +157,27 @@ func TestFetchAllPages(t *testing.T) { t.Fatalf("calls = %d, want %d", calls, maxPaginationPages) } }) + + t.Run("maximum items", func(t *testing.T) { + pages := map[string][]string{ + "": {"a", "b"}, + "cursor1": {"c", "d"}, + } + next := map[string]*string{"": strptr("cursor1"), "cursor1": strptr("cursor2")} + var gotCursors []string + + items, err := fetchAllPages(context.Background(), 3, func(_ context.Context, cursor string) ([]string, *string, error) { + gotCursors = append(gotCursors, cursor) + return pages[cursor], next[cursor], nil + }) + if err != nil { + t.Fatalf("error = %v", err) + } + if !reflect.DeepEqual(items, []string{"a", "b", "c"}) { + t.Fatalf("got %v, want [a b c]", items) + } + if !reflect.DeepEqual(gotCursors, []string{"", "cursor1"}) { + t.Fatalf("cursors = %v, want [ cursor1]", gotCursors) + } + }) } diff --git a/tangled/repo_list_issues.go b/tangled/repo_list_issues.go index 1be1121..76e8644 100644 --- a/tangled/repo_list_issues.go +++ b/tangled/repo_list_issues.go @@ -10,7 +10,7 @@ import ( // ListIssues fetches every issue for repoDid, following pagination // cursors until the listing is exhausted. func (t *Tangled) ListIssues(ctx context.Context, repoDid string, opts ListOpts) (*List, error) { - items, err := fetchAllPages(ctx, func(ctx context.Context, cursor string) ([]ListItem, *string, error) { + items, err := fetchAllPages(ctx, opts.MaxItems, func(ctx context.Context, cursor string) ([]ListItem, *string, error) { page, err := tangledlex.RepoListIssues(ctx, t.lexClient(), opts.Author, cursor, opts.limit(), opts.Order, opts.State, repoDid) if err != nil { return nil, nil, err diff --git a/tangled/repo_list_pulls.go b/tangled/repo_list_pulls.go index 524c598..ba57dd9 100644 --- a/tangled/repo_list_pulls.go +++ b/tangled/repo_list_pulls.go @@ -10,7 +10,7 @@ import ( // ListPulls fetches every pull request for repoDid, following pagination // cursors until the listing is exhausted. func (t *Tangled) ListPulls(ctx context.Context, repoDid string, opts ListOpts) (*List, error) { - items, err := fetchAllPages(ctx, func(ctx context.Context, cursor string) ([]ListItem, *string, error) { + items, err := fetchAllPages(ctx, opts.MaxItems, func(ctx context.Context, cursor string) ([]ListItem, *string, error) { page, err := tangledlex.RepoListPulls(ctx, t.lexClient(), opts.Author, cursor, opts.limit(), opts.Order, opts.State, repoDid) if err != nil { return nil, nil, err diff --git a/tangled/repo_list_repos.go b/tangled/repo_list_repos.go index 639cfcb..f90f74b 100644 --- a/tangled/repo_list_repos.go +++ b/tangled/repo_list_repos.go @@ -16,7 +16,7 @@ type RepoList struct { // ListRepos fetches every repo owned by ownerDid, following pagination // cursors until the listing is exhausted. func (t *Tangled) ListRepos(ctx context.Context, ownerDid string) (*RepoList, error) { - items, err := fetchAllPages(ctx, func(ctx context.Context, cursor string) ([]Repo, *string, error) { + items, err := fetchAllPages(ctx, 0, func(ctx context.Context, cursor string) ([]Repo, *string, error) { page, err := tangledlex.RepoListRepos(ctx, t.lexClient(), cursor, 100, "", ownerDid) if err != nil { return nil, nil, err