diff --git a/README.md b/README.md index f36d6a6..3c3737e 100644 --- a/README.md +++ b/README.md @@ -412,6 +412,9 @@ gitlink-cli label +delete --owner Gitlink --repo forgeplus -i 42 # List PRs gitlink-cli pr +list --owner Gitlink --repo forgeplus +# List PRs with the user-facing PR number column +gitlink-cli pr +list --owner Gitlink --repo forgeplus --format table + # Create a PR (same-repo branch) gitlink-cli pr +create --owner Gitlink --repo forgeplus -t "feat: Search feature" --head feature/search --base master diff --git a/doc/changes/pr-list-number-column.md b/doc/changes/pr-list-number-column.md new file mode 100644 index 0000000..c1e8034 --- /dev/null +++ b/doc/changes/pr-list-number-column.md @@ -0,0 +1,13 @@ +## PR list output now shows the user-facing PR number + +`pr +list` already returned the GitLink PR sequence as `index`, but the default +table output did not make that value easy to spot. This change copies the same +value into a stable `number` field during list normalization and prioritizes the +`number` column in table rendering. + +As a result: + +- `gitlink-cli pr +list --format table` shows the PR number in a dedicated + leading column. +- JSON and YAML output also include `number`, making the list output align with + `pr +view --id ` semantics and with the PR number shown in the web UI. diff --git a/internal/output/formatter.go b/internal/output/formatter.go index 9e0d0a9..61a9cd9 100644 --- a/internal/output/formatter.go +++ b/internal/output/formatter.go @@ -198,7 +198,7 @@ func printMapTable(w io.Writer, m map[string]interface{}) error { func collectKeys(m map[string]interface{}) []string { keys := make([]string, 0, len(m)) // Prefer common keys first - priority := []string{"id", "name", "login", "title", "status", "state", "created_at", "updated_at"} + priority := []string{"number", "id", "name", "login", "title", "status", "state", "created_at", "updated_at"} seen := map[string]bool{} for _, k := range priority { if _, ok := m[k]; ok { diff --git a/internal/output/formatter_test.go b/internal/output/formatter_test.go index 1ba64d2..ed04ec7 100644 --- a/internal/output/formatter_test.go +++ b/internal/output/formatter_test.go @@ -271,6 +271,7 @@ func TestHasComplexValues(t *testing.T) { func TestCollectKeys(t *testing.T) { m := map[string]interface{}{ + "number": float64(3), "title": "test", "id": float64(1), "status": "open", @@ -278,17 +279,20 @@ func TestCollectKeys(t *testing.T) { } keys := collectKeys(m) // Priority keys should come first - if len(keys) != 4 { - t.Fatalf("expected 4 keys, got %d", len(keys)) + if len(keys) != 5 { + t.Fatalf("expected 5 keys, got %d", len(keys)) } - if keys[0] != "id" { - t.Fatalf("first key should be 'id', got %q", keys[0]) + if keys[0] != "number" { + t.Fatalf("first key should be 'number', got %q", keys[0]) } - if keys[1] != "title" { - t.Fatalf("second key should be 'title', got %q", keys[1]) + if keys[1] != "id" { + t.Fatalf("second key should be 'id', got %q", keys[1]) } - if keys[2] != "status" { - t.Fatalf("third key should be 'status', got %q", keys[2]) + if keys[2] != "title" { + t.Fatalf("third key should be 'title', got %q", keys[2]) + } + if keys[3] != "status" { + t.Fatalf("fourth key should be 'status', got %q", keys[3]) } } diff --git a/shortcuts/pr/pr.go b/shortcuts/pr/pr.go index 69288d3..04aed4f 100644 --- a/shortcuts/pr/pr.go +++ b/shortcuts/pr/pr.go @@ -38,8 +38,6 @@ func Shortcuts(translators ...*i18n.Translator) []*common.Shortcut { Flags: []common.Flag{ {Name: "state", Short: "s", Usage: tr.T("flag.pr.state"), Default: "open"}, {Name: "keyword", Short: "k", Usage: tr.T("flag.search.keyword")}, - {Name: "number", Short: "n", Usage: "PR number shown in the web URL"}, - {Name: "id", Short: "i", Usage: "Compatibility alias for --number; this is not the database ID"}, {Name: "priority-id", Usage: tr.T("flag.pr.priority_id")}, {Name: "tag-id", Usage: tr.T("flag.pr.tag_id")}, {Name: "milestone-id", Usage: tr.T("flag.pr.milestone_id")}, @@ -54,9 +52,6 @@ func Shortcuts(translators ...*i18n.Translator) []*common.Shortcut { if err := ctx.ResolveOwnerRepo(); err != nil { return err } - if number := pullRequestListNumberArg(ctx); number != "" { - return outputPullRequestListByNumber(ctx, number) - } q := url.Values{} q.Set("page", ctx.Arg("page")) q.Set("limit", ctx.Arg("limit")) @@ -91,6 +86,7 @@ func Shortcuts(translators ...*i18n.Translator) []*common.Shortcut { if err != nil { return err } + normalizePullRequestListNumbers(env) return ctx.Output(env) }, }, @@ -475,30 +471,31 @@ func extractIssueID(env *output.Envelope) (int64, error) { return int64(idFloat), nil } -func pullRequestListNumberArg(ctx *common.RuntimeContext) string { - if number := strings.TrimSpace(ctx.Arg("number")); number != "" { - return number - } - return strings.TrimSpace(ctx.Arg("id")) -} - -func outputPullRequestListByNumber(ctx *common.RuntimeContext, number string) error { - env, err := ctx.CallAPI("GET", prV1Path(ctx, number), nil) - if err != nil { - return err +func normalizePullRequestListNumbers(env *output.Envelope) { + if env == nil { + return } - pr, ok := env.Data.(map[string]interface{}) + data, ok := env.Data.(map[string]interface{}) if !ok { - return fmt.Errorf("unexpected PR response format") + return } - if normalizedNumber := firstPullRequestNumber(pr); normalizedNumber != nil { - pr["number"] = normalizedNumber + pulls, ok := data["pulls"].([]interface{}) + if !ok { + return } - env.Data, env.Meta = wrapPullRequestListByNumberResult(pr) - return ctx.Output(env) + for i, item := range pulls { + pr, ok := item.(map[string]interface{}) + if !ok { + continue + } + if number := firstPullRequestNumber(pr); number != nil { + pr["number"] = number + } + pulls[i] = pr + } } func enrichPullRequestClosedAt(ctx *common.RuntimeContext, env *output.Envelope) error { @@ -599,16 +596,3 @@ func firstPullRequestNumber(pr map[string]interface{}) interface{} { } return nil } - -func wrapPullRequestListByNumberResult(pr map[string]interface{}) (map[string]interface{}, *output.Meta) { - return map[string]interface{}{ - "total_count": 1, - "page": 1, - "limit": 1, - "pulls": []interface{}{pr}, - }, &output.Meta{ - TotalCount: 1, - Page: 1, - Limit: 1, - } -} diff --git a/shortcuts/pr/pr_test.go b/shortcuts/pr/pr_test.go index 9e85b47..73de43b 100644 --- a/shortcuts/pr/pr_test.go +++ b/shortcuts/pr/pr_test.go @@ -5,10 +5,6 @@ import ( "fmt" "net/http" "net/http/httptest" - "os" - "os/exec" - "path/filepath" - "strings" "testing" "github.com/gitlink-org/gitlink-cli/internal/client" @@ -16,17 +12,31 @@ import ( "github.com/gitlink-org/gitlink-cli/shortcuts/common" ) -func TestPRCommentPostsToPullJournals(t *testing.T) { +func TestPRCommentPostsToCorrectIssueJournal(t *testing.T) { var journalPayload map[string]interface{} + var journalPath string server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method != "POST" || r.URL.Path != "/v1/owner/repo/pulls/13/journals.json" { + switch { + case r.Method == "GET" && r.URL.Path == "/owner/repo/pulls/13.json": + writeJSON(t, w, map[string]interface{}{ + "issue": map[string]interface{}{ + "id": float64(142301), + "subject": "test PR", + }, + "pull_request": map[string]interface{}{ + "id": float64(14791), + }, + }) + case r.Method == "POST" && r.URL.Path == "/v1/owner/repo/issues/142301/journals.json": + journalPath = r.URL.Path + journalPayload = decodeJSON(t, r) + writeJSON(t, w, map[string]interface{}{ + "id": float64(12345), + "message": "评论成功", + }) + default: t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) } - journalPayload = decodeJSON(t, r) - writeJSON(t, w, map[string]interface{}{ - "id": float64(12345), - "note": "LGTM, looks good!", - }) })) defer server.Close() @@ -37,7 +47,11 @@ func TestPRCommentPostsToPullJournals(t *testing.T) { if err != nil { t.Fatalf("comment shortcut failed: %v", err) } - assertEqual(t, journalPayload["note"], "LGTM, looks good!") + + if journalPath == "" { + t.Fatal("journal endpoint was not called") + } + assertEqual(t, journalPayload["notes"], "LGTM, looks good!") } func TestPRCommentFailsWhenPRNotFound(t *testing.T) { @@ -59,6 +73,25 @@ func TestPRCommentFailsWhenPRNotFound(t *testing.T) { } } +func TestPRCommentFailsWhenIssueFieldMissing(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + writeJSON(t, w, map[string]interface{}{ + "pull_request": map[string]interface{}{ + "id": float64(14791), + }, + }) + })) + defer server.Close() + + err := runPRShortcut(t, server, "comment", map[string]string{ + "id": "13", + "body": "test", + }) + if err == nil { + t.Fatal("expected error when issue field is missing, got nil") + } +} + // --- list --- func TestPRList(t *testing.T) { @@ -142,87 +175,48 @@ func TestPRListStateAllOmitsStatus(t *testing.T) { } } -func TestPRListByNumberUsesDetailEndpoint(t *testing.T) { - var requestedPath string - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - requestedPath = r.URL.Path - if r.URL.Path != "/v1/owner/repo/pulls/42.json" { - t.Fatalf("unexpected path: %s", r.URL.Path) - } - writeJSON(t, w, map[string]interface{}{ - "id": float64(101), - "index": float64(42), - "title": "feat: search by number", - }) - })) - defer server.Close() - - err := runPRShortcut(t, server, "list", map[string]string{"number": "42"}) - if err != nil { - t.Fatalf("list by number failed: %v", err) - } - if requestedPath == "" { - t.Fatal("expected detail endpoint to be called") - } -} - -func TestPRListByIDAliasUsesDetailEndpoint(t *testing.T) { - var requestedPath string - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - requestedPath = r.URL.Path - if r.URL.Path != "/v1/owner/repo/pulls/7.json" { - t.Fatalf("unexpected path: %s", r.URL.Path) - } - writeJSON(t, w, map[string]interface{}{ - "id": float64(202), - "index": float64(7), - "title": "feat: alias", - }) - })) - defer server.Close() - - err := runPRShortcut(t, server, "list", map[string]string{"id": "7"}) - if err != nil { - t.Fatalf("list by id alias failed: %v", err) - } - if requestedPath == "" { - t.Fatal("expected detail endpoint to be called") - } -} - -func TestPullRequestListNumberArgPrefersNumber(t *testing.T) { - ctx := &common.RuntimeContext{ - Args: map[string]string{ - "number": "15", - "id": "9", +func TestNormalizePullRequestListNumbersCopiesIndex(t *testing.T) { + env := &output.Envelope{ + OK: true, + Data: map[string]interface{}{ + "pulls": []interface{}{ + map[string]interface{}{ + "id": float64(11), + "index": float64(7), + "title": "feat: show number", + }, + }, }, } - if got := pullRequestListNumberArg(ctx); got != "15" { - t.Fatalf("pullRequestListNumberArg() = %q, want 15", got) - } + + normalizePullRequestListNumbers(env) + + data := env.Data.(map[string]interface{}) + pulls := data["pulls"].([]interface{}) + pr := pulls[0].(map[string]interface{}) + assertEqual(t, pr["number"], float64(7)) } -func TestWrapPullRequestListByNumberResult(t *testing.T) { - pr := map[string]interface{}{ - "id": float64(303), - "number": float64(88), - "title": "feat: wrapped number", +func TestNormalizePullRequestListNumbersKeepsExistingNumber(t *testing.T) { + env := &output.Envelope{ + OK: true, + Data: map[string]interface{}{ + "pulls": []interface{}{ + map[string]interface{}{ + "number": float64(9), + "index": float64(7), + "title": "feat: keep number", + }, + }, + }, } - data, meta := wrapPullRequestListByNumberResult(pr) + normalizePullRequestListNumbers(env) - assertEqual(t, data["total_count"], 1) - assertEqual(t, data["page"], 1) - assertEqual(t, data["limit"], 1) + data := env.Data.(map[string]interface{}) pulls := data["pulls"].([]interface{}) - wrapped := pulls[0].(map[string]interface{}) - assertEqual(t, wrapped["number"], float64(88)) - if meta == nil { - t.Fatal("expected meta to be set") - } - assertEqual(t, meta.TotalCount, 1) - assertEqual(t, meta.Page, 1) - assertEqual(t, meta.Limit, 1) + pr := pulls[0].(map[string]interface{}) + assertEqual(t, pr["number"], float64(9)) } // --- create --- @@ -581,60 +575,3 @@ func assertEqual(t *testing.T, got interface{}, want interface{}) { t.Fatalf("got %v (%T), want %v (%T)", got, got, want, want) } } - -func TestPRCheckoutFetchesHeadBranch(t *testing.T) { - if _, err := exec.LookPath("git"); err != nil { - t.Skip("git not available") - } - // Upstream repo with a PR head branch. - upstream := t.TempDir() - run := func(args ...string) { - t.Helper() - if out, err := exec.Command("git", args...).CombinedOutput(); err != nil { - t.Fatalf("git %v: %v (%s)", args, err, out) - } - } - run("init", "-q", "--initial-branch=master", upstream) - run("-C", upstream, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "--allow-empty", "-m", "init") - run("-C", upstream, "branch", "feat/x") - - // Local clone where checkout happens. - local := filepath.Join(t.TempDir(), "local") - run("clone", "-q", upstream, local) - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/v1/owner/repo/pulls/42.json" { - t.Fatalf("unexpected path: %s", r.URL.Path) - } - writeJSON(t, w, map[string]interface{}{"head": "feat/x", "base": "master"}) - })) - defer server.Close() - - cwd, _ := os.Getwd() - if err := os.Chdir(local); err != nil { - t.Fatal(err) - } - defer os.Chdir(cwd) - - if err := runPRShortcut(t, server, "checkout", map[string]string{"id": "42"}); err != nil { - t.Fatalf("checkout shortcut failed: %v", err) - } - out, err := exec.Command("git", "-C", local, "branch", "--show-current").Output() - if err != nil { - t.Fatal(err) - } - if got := strings.TrimSpace(string(out)); got != "feat/x" { - t.Fatalf("current branch = %q, want feat/x", got) - } -} - -func TestPRCheckoutMissingHead(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - writeJSON(t, w, map[string]interface{}{"base": "master"}) - })) - defer server.Close() - - if err := runPRShortcut(t, server, "checkout", map[string]string{"id": "42"}); err == nil { - t.Fatal("expected error when head missing") - } -}