From 2ef60907344cedcc2e5b48cf884ca048a633124c Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 11 Oct 2026 22:34:47 +0200 Subject: [PATCH] feat(app): show the usage line on argument errors and allow -L up to 1000 Positional argument errors name what is wrong from the command's usage, then print the usage line and where to find help: missing issue number Usage: tea issue view [flags] Run 'tea issue view --help' for more information. instead of "accepts 1 arg(s), received 0". Resource numbers accept #N. --limit gains the gh shorthand -L and accepts up to 1000 results; the list services already page through the server's smaller pages to fill a window. Signed-off-by: bircni --- internal/app/args.go | 110 ++++++++++++++++++++++++++++++-- internal/app/args_test.go | 55 ++++++++++++++++ internal/app/validation_test.go | 14 ++-- 3 files changed, 167 insertions(+), 12 deletions(-) diff --git a/internal/app/args.go b/internal/app/args.go index 072bbe14..0da1fbf2 100644 --- a/internal/app/args.go +++ b/internal/app/args.go @@ -18,8 +18,9 @@ import ( "github.com/spf13/pflag" ) +// number parses a resource number written as N or #N. func number(raw string) (int64, error) { - n, err := strconv.ParseInt(raw, 10, 64) + n, err := strconv.ParseInt(strings.TrimPrefix(raw, "#"), 10, 64) if err != nil || n < 1 { return 0, clierr.Wrap(clierr.Usage, fmt.Errorf("invalid resource number %q", raw)) } @@ -83,9 +84,13 @@ func oneOf(flag, value string, allowed ...string) error { return clierr.Wrap(clierr.Usage, fmt.Errorf("--%s must be %s or %s", flag, choices, allowed[last])) } -// pageFlags registers --limit and --page, which reject out-of-range values while parsing. +// maxLimit bounds --limit; list services page through the server's smaller pages to reach it. +const maxLimit = 1000 + +// pageFlags registers -L/--limit and --page, which reject out-of-range values while parsing. func pageFlags(cmd *cobra.Command, limit, page *int) { - positiveIntVar(cmd, limit, "limit", 30, 100, "Maximum results (1..100)") + *limit = 30 + cmd.Flags().VarP(positiveInt{limit, maxLimit}, "limit", "L", "Maximum number of results (1..1000)") positiveIntVar(cmd, page, "page", 1, math.MaxInt, "Result page") } @@ -194,13 +199,108 @@ func argsExact(n int) cobra.PositionalArgs { return usageArgs(cobra.ExactArgs(n)) } -// usageArgs reports positional argument errors with the usage exit code. +// usageArgs reports positional argument errors with the usage exit code, the usage line and a +// pointer to the command's help. func usageArgs(validate cobra.PositionalArgs) cobra.PositionalArgs { return func(cmd *cobra.Command, args []string) error { - return clierr.Wrap(clierr.Usage, validate(cmd, args)) + err := validate(cmd, args) + if err == nil { + return nil + } + return clierr.Wrap(clierr.Usage, &argumentError{problem: argumentProblem(cmd, args, err), cmd: cmd}) } } +// argumentError reports a positional argument problem followed by the command's usage. +type argumentError struct { + problem string + cmd *cobra.Command +} + +func (e *argumentError) Error() string { + more := "for more information" + if e.cmd.Example != "" { + more = "for examples" + } + return fmt.Sprintf("%s\nUsage: %s\nRun '%s --help' %s.", e.problem, e.cmd.UseLine(), e.cmd.CommandPath(), more) +} + +// argumentProblem names what is wrong with args from the arguments in the command's Use, such +// as "missing issue number", falling back to the validator's message. +func argumentProblem(cmd *cobra.Command, args []string, err error) string { + arguments := usageArguments(cmd.Use) + required, variadic := 0, false + for _, argument := range arguments { + if !strings.HasPrefix(argument, "[") { + required++ + } + variadic = variadic || strings.HasSuffix(strings.TrimRight(argument, "]}>"), "...") + } + switch { + case len(args) < required: + return "missing " + argumentName(cmd, arguments[len(args)]) + case len(arguments) == 0 && len(args) > 0: + return fmt.Sprintf("unexpected argument %q", args[0]) + case !variadic && len(arguments) > 0 && len(args) > len(arguments): + return "too many arguments" + } + return err.Error() +} + +// usageArguments splits the arguments of a Use line, such as "view ", into its +// top-level groups: "", "[ | ]" or "...". [flags] is not one. +func usageArguments(use string) []string { + _, rest, _ := strings.Cut(use, " ") + var arguments []string + depth, start := 0, -1 + for i, r := range rest { + switch { + case strings.ContainsRune("<[{", r): + if depth == 0 { + start = i + } + depth++ + case strings.ContainsRune(">]}", r): + depth-- + } + if depth == 0 && start >= 0 && (i == len(rest)-1 || rest[i+1] == ' ') { + if argument := rest[start : i+1]; argument != "[flags]" { + arguments = append(arguments, argument) + } + start = -1 + } + } + return arguments +} + +// argumentName words a Use argument for an error: "" is "asset ID", and a bare +// "" or "" names the command group's resource, as in "issue number". +func argumentName(cmd *cobra.Command, argument string) string { + first, _, _ := strings.Cut(argument, "|") + name := strings.Trim(first, "<>[]{}. ") + words := strings.Split(name, "-") + for i, word := range words { + if word == "id" { + words[i] = "ID" + } + } + text := strings.Join(words, " ") + if name != "number" && name != "id" && name != "id-or-name" { + return text + } + group := cmd + for group.HasParent() && group.Parent().HasParent() { + group = group.Parent() + } + if group == cmd { + return text + } + if noun := group.Name(); noun == "pr" { + return "pull request " + text + } + return group.Name() + " " + text +} + // localFlagsChanged reports whether any command-specific flag was supplied. func localFlagsChanged(cmd *cobra.Command) bool { changed := false diff --git a/internal/app/args_test.go b/internal/app/args_test.go index bfed89a7..0c591820 100644 --- a/internal/app/args_test.go +++ b/internal/app/args_test.go @@ -51,3 +51,58 @@ func TestParseTimeRangeRejectsOnlyInvertedBounds(t *testing.T) { require.Equal(t, tc.wantErr, err != nil, tc.since+".."+tc.until) } } + +func TestNumberAcceptsHashPrefix(t *testing.T) { + for raw, want := range map[string]int64{"12": 12, "#12": 12} { + n, err := number(raw) + require.NoError(t, err, raw) + require.Equal(t, want, n, raw) + } + for _, raw := range []string{"#", "##1", "0", "#-1", "1#", "abc"} { + _, err := number(raw) + require.ErrorContains(t, err, "invalid resource number", raw) + } +} + +func TestUsageArgumentsSplitTopLevelGroups(t *testing.T) { + for use, want := range map[string][]string{ + "list": nil, + "list [flags]": nil, + "view ": {""}, + "view [number]": {"[number]"}, + "close ...": {"..."}, + "clone [directory]": {"", "[directory]"}, + "diff [ | | ]": {"[ | | ]"}, + "download [flags]": {"", ""}, + "upload ... [flags]": {"", "..."}, + } { + require.Equal(t, want, usageArguments(use), use) + } +} + +func TestArgumentErrorsShowTheUsageLine(t *testing.T) { + for _, tc := range []struct { + args []string + want string + }{ + {[]string{"issue", "view"}, "missing issue number\nUsage: tea issue view [flags]\nRun 'tea issue view --help' for more information."}, + {[]string{"issue", "view", "1", "2"}, "too many arguments\nUsage: tea issue view [flags]"}, + {[]string{"issue", "list", "extra"}, "unexpected argument \"extra\"\nUsage: tea issue list [flags]"}, + {[]string{"pr", "checkout"}, "missing pull request number\nUsage: tea pr checkout [flags]"}, + {[]string{"run", "view"}, "missing run ID\nUsage: tea run view [flags]"}, + {[]string{"release", "asset", "download", "v1"}, "missing asset ID\nUsage: tea release asset download [flags]"}, + {[]string{"repo", "clone"}, "missing repository\nUsage: tea repo clone [directory] [flags]"}, + } { + requireUsageBeforeRequests(t, tc.want, tc.args...) + } +} + +func TestLimitAcceptsShorthandUpToOneThousand(t *testing.T) { + var limit, page int + cmd := &cobra.Command{Use: "list"} + pageFlags(cmd, &limit, &page) + require.Equal(t, 30, limit) + require.NoError(t, cmd.Flags().Parse([]string{"-L", "1000"})) + require.Equal(t, 1000, limit) + require.ErrorContains(t, cmd.Flags().Parse([]string{"-L", "1001"}), "must be 1..1000") +} diff --git a/internal/app/validation_test.go b/internal/app/validation_test.go index 674f4119..bc2db19e 100644 --- a/internal/app/validation_test.go +++ b/internal/app/validation_test.go @@ -67,7 +67,7 @@ func TestUsageErrorsPrecedeRequests(t *testing.T) { {[]string{"issue", "edit", "1", "invalid", "--title", "Updated"}, "invalid resource number \"invalid\""}, {[]string{"issue", "edit", "1", "2", "--editor"}, "--editor requires one issue"}, {[]string{"issue", "edit", "1", "01", "--title", "Updated", "--no-input"}, "resource 1 is selected more than once"}, - {[]string{"issue", "list", "--limit", "0"}, `"--limit" flag: must be 1..100`}, + {[]string{"issue", "list", "--limit", "0"}, `"-L, --limit" flag: must be 1..1000`}, {[]string{"issue", "list", "--page", "0"}, `"--page" flag: must be a positive integer`}, {[]string{"issue", "list", "--label", "bug, "}, "label names cannot be empty"}, {[]string{"label", "create", "--name", "bug", "--color", "wrong"}, "--color requires six hexadecimal digits"}, @@ -83,11 +83,11 @@ func TestUsageErrorsPrecedeRequests(t *testing.T) { {[]string{"milestone", "edit", "9", "--title", " \t "}, "milestone title cannot be empty"}, {[]string{"milestone", "issue", "add", "7", "1", "01", "--no-input"}, "resource 1 is selected more than once"}, {[]string{"milestone", "issue", "remove", "7", "1", "1", "--no-input"}, "resource 1 is selected more than once"}, - {[]string{"milestone", "list", "--limit", "101"}, `"--limit" flag: must be 1..100`}, + {[]string{"milestone", "list", "--limit", "1001"}, `"-L, --limit" flag: must be 1..1000`}, {[]string{"milestone", "list", "--state", "merged"}, "--state must be open, closed, or all"}, {[]string{"notification", "list", "--status", "invalid"}, "--status must contain unread, read, or pinned"}, {[]string{"notification", "list", "--type", "invalid"}, "--type must contain issue, pull, commit, or repository"}, - {[]string{"notification", "list", "--limit", "many"}, `"--limit" flag: must be 1..100`}, + {[]string{"notification", "list", "--limit", "many"}, `"-L, --limit" flag: must be 1..1000`}, {[]string{"notification", "list", "--mine", "-R", "alice/project"}, "--mine and --repo are mutually exclusive"}, {[]string{"notification", "list", "--since="}, "invalid --since: use a date, RFC3339 timestamp, or duration such as 24h"}, {[]string{"notification", "list", "--since=1h", "--before=2h"}, "--since must not be later than --before"}, @@ -102,7 +102,7 @@ func TestUsageErrorsPrecedeRequests(t *testing.T) { {[]string{"org", "create", "team", "--visibility", "unknown"}, "--visibility must be public, limited, or private"}, {[]string{"org", "edit", "team"}, "supply organization fields to edit"}, {[]string{"org", "list", "--all", "--username", "alice"}, "--all and --username are mutually exclusive"}, - {[]string{"org", "member", "list", "team", "--limit", "0"}, `"--limit" flag: must be 1..100`}, + {[]string{"org", "member", "list", "team", "--limit", "0"}, `"-L, --limit" flag: must be 1..1000`}, {[]string{"pr", "attachment", "download", "1", "1"}, "supply --output-file, or --output-file - for stdout"}, {[]string{"pr", "attachment", "download", "1", "1", "--json"}, "attachment downloads are binary"}, {[]string{"pr", "attachment", "download", "1", "1", "--output-file", "-", "--json=id"}, "attachment downloads are binary"}, @@ -131,7 +131,7 @@ func TestUsageErrorsPrecedeRequests(t *testing.T) { {[]string{"pr", "list", "--sort", "unknown"}, "--sort must be oldest, recentupdate, recentclose, leastupdate, mostcomment, leastcomment, or priority"}, {[]string{"pr", "list", "--milestone", "-1"}, "--milestone must be a nonnegative ID"}, {[]string{"pr", "list", "--state", "unknown"}, "--state must be open, closed, or all"}, - {[]string{"pr", "list", "--limit", "0"}, `"--limit" flag: must be 1..100`}, + {[]string{"pr", "list", "--limit", "0"}, `"-L, --limit" flag: must be 1..1000`}, {[]string{"pr", "review", "1", "--no-input"}, "missing required input: supply --body"}, {[]string{"pr", "review", "1", "--body="}, "review body cannot be empty without inline comments"}, {[]string{"pr", "review", "1", "--event=unknown"}, "--event must be APPROVED, REQUEST_CHANGES, COMMENT, or PENDING"}, @@ -167,7 +167,7 @@ func TestUsageErrorsPrecedeRequests(t *testing.T) { {[]string{"repo", "edit", "--default-branch="}, "default branch cannot be empty"}, {[]string{"repo", "archive", "alice/project", "-R", "alice/other", "--yes"}, "supply the repository as an argument or --repo, not both"}, {[]string{"repo", "view", "alice/project", "--repo", "alice/other"}, "supply the repository as an argument or --repo, not both"}, - {[]string{"repo", "view", "alice/project", "alice/other"}, "accepts at most 1 arg(s)"}, + {[]string{"repo", "view", "alice/project", "alice/other"}, "too many arguments\nUsage: tea repo view [repository] [flags]"}, {[]string{"repo", "list", "--owner", "account", "--starred"}, "--owner, --starred, and --watched select mutually exclusive collections"}, {[]string{"repo", "list", "--starred", "--watched"}, "--owner, --starred, and --watched select mutually exclusive collections"}, {[]string{"repo", "list", "--type", "invalid"}, "--type must be fork, source, or mirror"}, @@ -189,7 +189,7 @@ func TestUsageErrorsPrecedeRequests(t *testing.T) { {[]string{"run", "list", "--since=-1h"}, "invalid --since: relative duration cannot be negative"}, {[]string{"run", "list", "--until="}, "invalid --until: use a date, RFC3339 timestamp, or duration such as 24h"}, {[]string{"run", "list", "--since=1h", "--until=2h"}, "--since must not be later than --until"}, - {[]string{"run", "list", "--limit", "0"}, `"--limit" flag: must be 1..100`}, + {[]string{"run", "list", "--limit", "0"}, `"-L, --limit" flag: must be 1..1000`}, {[]string{"search", "issue", "query", "--state", "invalid"}, "--state must be open, closed, or all"}, {[]string{"search", "issue", "query", "--author", "alice", "--authored"}, "--author and --authored are mutually exclusive"}, {[]string{"search", "issue", "--label", ","}, "label names cannot be empty"},