From 6e71ff994104208ceda2130fbbc09080796a6b39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9E=97=E6=99=A8=20=28Leo=20Cheng=29?= Date: Wed, 8 Jul 2026 12:11:47 +0800 Subject: [PATCH] feat(pr): add pr +edit to update an open pull request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: 林晨 (Leo Cheng) --- doc/changes/pr-edit.md | 18 ++++ internal/i18n/locales/en-US.json | 2 + internal/i18n/locales/zh-CN.json | 2 + shortcuts/pr/pr.go | 142 ++++++++++++++++++++++++++++++ shortcuts/pr/pr_test.go | 145 +++++++++++++++++++++++++++++++ 5 files changed, 309 insertions(+) create mode 100644 doc/changes/pr-edit.md diff --git a/doc/changes/pr-edit.md b/doc/changes/pr-edit.md new file mode 100644 index 0000000..a733448 --- /dev/null +++ b/doc/changes/pr-edit.md @@ -0,0 +1,18 @@ +# PR Edit Shortcut + +This change adds a `pr +edit` verb, bringing pull requests in line with `issue`, `label`, `release`, `milestone`, and `webhook`, which already expose an edit/update command. + +## Commands + +- Add `pr +edit` for `PUT /api/{owner}/{repo}/pulls/{index}.json`. + +## Behavior + +- `pr +edit` first GETs the current PR (the same `/pulls/{index}` path used by `pr +view`), then merges the requested changes onto its current values before the PUT. The update endpoint requires `title`, `body`, `head`, `base`, `issue_tag_ids`, and `receivers_login` together, so unspecified flags fall back to the existing values to avoid clobbering them. +- Flags: `--title`, `--body`, `--base`, `--head`, and `--tag-ids` (comma-separated). At least one is required. +- `--tag-ids` replaces the tag set when given; otherwise the PR's current tag IDs are preserved. +- PR detail fields are read defensively across the top-level, `pull_request`, and `issue` scopes, matching how the health checks parse the same endpoint. + +## Verification + +- Unit tests cover field preservation, tag-id override, the nested `pull_request` response shape, the missing-field guard, and the HTTP error path. diff --git a/internal/i18n/locales/en-US.json b/internal/i18n/locales/en-US.json index 0739395..628320e 100644 --- a/internal/i18n/locales/en-US.json +++ b/internal/i18n/locales/en-US.json @@ -56,6 +56,7 @@ "cmd.pr.comment.short": "Add a comment to a pull request", "cmd.pr.create.short": "Create a pull request", "cmd.pr.diff.short": "Show diff for a pull request", + "cmd.pr.edit.short": "Edit a pull request while preserving unspecified fields", "cmd.pr.files.short": "List changed files in a pull request", "cmd.pr.list.short": "List pull requests", "cmd.pr.merge.short": "Merge a pull request", @@ -194,6 +195,7 @@ "flag.pr.reviewer_id": "Reviewer user ID", "flag.pr.state": "Filter: open, merged, closed", "flag.pr.tag_id": "Issue tag ID", + "flag.pr.tag_ids": "Comma-separated issue tag IDs", "flag.pr.title": "PR title", "flag.pr.version_id": "Patchset version ID", "flag.profile.end_time": "End time (Unix timestamp)", diff --git a/internal/i18n/locales/zh-CN.json b/internal/i18n/locales/zh-CN.json index 2e6fc4d..6afc9ed 100644 --- a/internal/i18n/locales/zh-CN.json +++ b/internal/i18n/locales/zh-CN.json @@ -56,6 +56,7 @@ "cmd.pr.comment.short": "给拉取请求添加评论", "cmd.pr.create.short": "创建拉取请求", "cmd.pr.diff.short": "显示拉取请求 diff", + "cmd.pr.edit.short": "编辑拉取请求并保留未指定字段", "cmd.pr.files.short": "列出拉取请求中的变更文件", "cmd.pr.list.short": "列出拉取请求", "cmd.pr.merge.short": "合并拉取请求", @@ -194,6 +195,7 @@ "flag.pr.reviewer_id": "评审人用户 ID", "flag.pr.state": "筛选：open、merged、closed", "flag.pr.tag_id": "议题标签 ID", + "flag.pr.tag_ids": "逗号分隔的议题标签 ID", "flag.pr.title": "PR 标题", "flag.pr.version_id": "补丁集版本 ID", "flag.profile.end_time": "结束时间（Unix 时间戳）", diff --git a/shortcuts/pr/pr.go b/shortcuts/pr/pr.go index 03f537f..24768ed 100644 --- a/shortcuts/pr/pr.go +++ b/shortcuts/pr/pr.go @@ -1,8 +1,10 @@ package pr import ( + "encoding/json" "fmt" "net/url" + "strconv" "strings" "github.com/gitlink-org/gitlink-cli/internal/i18n" @@ -144,6 +146,52 @@ func Shortcuts(translators ...*i18n.Translator) []*common.Shortcut { return ctx.Output(env) }, }, + { + Name: "edit", + Description: tr.T("cmd.pr.edit.short"), + Flags: []common.Flag{ + {Name: "id", Short: "i", Usage: tr.T("flag.pr.id"), Required: true}, + {Name: "title", Short: "t", Usage: tr.T("flag.pr.title")}, + {Name: "body", Short: "b", Usage: tr.T("flag.pr.body")}, + {Name: "base", Usage: tr.T("flag.pr.base")}, + {Name: "head", Usage: tr.T("flag.pr.head")}, + {Name: "tag-ids", Usage: tr.T("flag.pr.tag_ids")}, + }, + Run: func(ctx *common.RuntimeContext) error { + if err := ctx.ResolveOwnerRepo(); err != nil { + return err + } + id, err := ctx.RequireArg("id") + if err != nil { + return err + } + if ctx.Arg("title") == "" && ctx.Arg("body") == "" && ctx.Arg("base") == "" && + ctx.Arg("head") == "" && ctx.Arg("tag-ids") == "" { + return fmt.Errorf("at least one of --title, --body, --base, --head, or --tag-ids is required") + } + + // The update endpoint requires title, body, head and base together, + // so merge the requested changes onto the PR's current values to + // avoid clobbering fields the caller did not pass. + path := fmt.Sprintf("%s/pulls/%s", ctx.RepoPath(), id) + current, err := ctx.CallAPI("GET", path, nil) + if err != nil { + return fmt.Errorf("fetch PR: %w", err) + } + payload := pullRequestEditPayload(ctx, current) + if payload["title"] == "" { + return fmt.Errorf("could not resolve PR title for #%s; pass --title explicitly", id) + } + if payload["head"] == "" || payload["base"] == "" { + return fmt.Errorf("could not resolve PR head/base branch for #%s; pass --head and --base explicitly", id) + } + env, err := ctx.CallAPI("PUT", path, payload) + if err != nil { + return err + } + return ctx.Output(env) + }, + }, { Name: "merge", Description: tr.T("cmd.pr.merge.short"), @@ -470,6 +518,100 @@ func extractIssueID(env *output.Envelope) (int64, error) { return int64(idFloat), nil } +// pullRequestEditPayload merges the caller's flags over the PR's current values. +// The PUT endpoint requires every field, so unspecified flags fall back to the +// values read from the prior GET to avoid wiping them. +func pullRequestEditPayload(ctx *common.RuntimeContext, current *output.Envelope) map[string]interface{} { + data, _ := current.Data.(map[string]interface{}) + return map[string]interface{}{ + "title": firstNonEmpty(ctx.Arg("title"), pullRequestField(data, "title", "subject", "name")), + "body": firstNonEmpty(ctx.Arg("body"), pullRequestField(data, "body", "description")), + "head": firstNonEmpty(ctx.Arg("head"), pullRequestField(data, "head", "pull_request_head")), + "base": firstNonEmpty(ctx.Arg("base"), pullRequestField(data, "base", "pull_request_base")), + "issue_tag_ids": pullRequestEditTagIDs(ctx, data), + "receivers_login": []string{}, + } +} + +// pullRequestEditTagIDs uses the explicitly requested --tag-ids when present, +// otherwise preserves the tag IDs already attached to the PR. +func pullRequestEditTagIDs(ctx *common.RuntimeContext, data map[string]interface{}) []string { + if raw := ctx.Arg("tag-ids"); raw != "" { + ids := make([]string, 0) + for _, part := range strings.Split(raw, ",") { + if trimmed := strings.TrimSpace(part); trimmed != "" { + ids = append(ids, trimmed) + } + } + return ids + } + for _, scope := range pullRequestFieldScopes(data) { + tags, ok := scope["issue_tags"].([]interface{}) + if !ok { + continue + } + ids := make([]string, 0, len(tags)) + for _, raw := range tags { + tag, ok := raw.(map[string]interface{}) + if !ok { + continue + } + if id := tagIDString(tag["id"]); id != "" { + ids = append(ids, id) + } + } + return ids + } + return []string{} +} + +// pullRequestField reads the first non-empty string among the given keys across +// the possible response scopes. The detail endpoint sometimes nests PR fields +// under pull_request/issue rather than at the top level. +func pullRequestField(data map[string]interface{}, keys ...string) string { + for _, scope := range pullRequestFieldScopes(data) { + for _, key := range keys { + if v := stringField(scope, key); v != "" { + return v + } + } + } + return "" +} + +func pullRequestFieldScopes(data map[string]interface{}) []map[string]interface{} { + scopes := []map[string]interface{}{data} + if pr, ok := data["pull_request"].(map[string]interface{}); ok { + scopes = append(scopes, pr) + } + if issue, ok := data["issue"].(map[string]interface{}); ok { + scopes = append(scopes, issue) + } + return scopes +} + +func firstNonEmpty(values ...string) string { + for _, v := range values { + if v != "" { + return v + } + } + return "" +} + +func tagIDString(v interface{}) string { + switch id := v.(type) { + case string: + return id + case float64: + return strconv.FormatInt(int64(id), 10) + case json.Number: + return id.String() + default: + return "" + } +} + func enrichPullRequestClosedAt(ctx *common.RuntimeContext, env *output.Envelope) error { data, ok := env.Data.(map[string]interface{}) if !ok { diff --git a/shortcuts/pr/pr_test.go b/shortcuts/pr/pr_test.go index eece6d9..2798122 100644 --- a/shortcuts/pr/pr_test.go +++ b/shortcuts/pr/pr_test.go @@ -351,6 +351,151 @@ func TestPRDiff(t *testing.T) { } } +// --- edit --- + +func editServer(t *testing.T, current map[string]interface{}, put *map[string]interface{}) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/owner/repo/pulls/42.json" { + t.Fatalf("unexpected path: %s", r.URL.Path) + } + switch r.Method { + case "GET": + writeJSON(t, w, current) + case "PUT": + *put = decodeJSON(t, r) + writeJSON(t, w, map[string]interface{}{}) + default: + t.Fatalf("unexpected method: %s", r.Method) + } + })) +} + +func TestPREditPreservesUnspecifiedFields(t *testing.T) { + var put map[string]interface{} + server := editServer(t, map[string]interface{}{ + "id": float64(42), + "title": "original title", + "body": "original body", + "head": "feature/x", + "base": "master", + "issue": map[string]interface{}{ + "id": float64(105), + "issue_tags": []interface{}{ + map[string]interface{}{"id": float64(7), "name": "bug"}, + map[string]interface{}{"id": float64(9), "name": "urgent"}, + }, + }, + }, &put) + defer server.Close() + + err := runPRShortcut(t, server, "edit", map[string]string{ + "id": "42", + "body": "updated body", + "base": "develop", + }) + if err != nil { + t.Fatalf("edit failed: %v", err) + } + if put == nil { + t.Fatal("PUT was not called") + } + assertEqual(t, put["title"], "original title") + assertEqual(t, put["body"], "updated body") + assertEqual(t, put["head"], "feature/x") + assertEqual(t, put["base"], "develop") + + tags, ok := put["issue_tag_ids"].([]interface{}) + if !ok || len(tags) != 2 || fmt.Sprintf("%v", tags[0]) != "7" || fmt.Sprintf("%v", tags[1]) != "9" { + t.Fatalf("expected preserved tag ids [7 9], got %v", put["issue_tag_ids"]) + } + if _, ok := put["receivers_login"].([]interface{}); !ok { + t.Fatalf("expected receivers_login array, got %v", put["receivers_login"]) + } +} + +func TestPREditTagIDsOverride(t *testing.T) { + var put map[string]interface{} + server := editServer(t, map[string]interface{}{ + "title": "keep", + "body": "keep", + "head": "src", + "base": "master", + "issue": map[string]interface{}{ + "issue_tags": []interface{}{ + map[string]interface{}{"id": float64(7)}, + }, + }, + }, &put) + defer server.Close() + + err := runPRShortcut(t, server, "edit", map[string]string{ + "id": "42", + "tag-ids": "3, 5", + }) + if err != nil { + t.Fatalf("edit failed: %v", err) + } + tags, ok := put["issue_tag_ids"].([]interface{}) + if !ok || len(tags) != 2 || fmt.Sprintf("%v", tags[0]) != "3" || fmt.Sprintf("%v", tags[1]) != "5" { + t.Fatalf("expected tag ids [3 5], got %v", put["issue_tag_ids"]) + } +} + +// The non-v1 detail endpoint nests the branch names under pull_request.head / +// pull_request.base (branch-name strings), which is the shape the live API +// actually returns; editing title-only must still preserve them. +func TestPREditReadsNestedPullRequestFields(t *testing.T) { + var put map[string]interface{} + server := editServer(t, map[string]interface{}{ + "pull_request": map[string]interface{}{ + "title": "nested title", + "body": "nested body", + "head": "topic", + "base": "main", + }, + "issue": map[string]interface{}{"id": float64(1)}, + }, &put) + defer server.Close() + + err := runPRShortcut(t, server, "edit", map[string]string{ + "id": "42", + "title": "new title", + }) + if err != nil { + t.Fatalf("edit failed: %v", err) + } + assertEqual(t, put["title"], "new title") + assertEqual(t, put["body"], "nested body") + assertEqual(t, put["head"], "topic") + assertEqual(t, put["base"], "main") +} + +func TestPREditRequiresAtLeastOneField(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatalf("no request expected, got %s %s", r.Method, r.URL.Path) + })) + defer server.Close() + + err := runPRShortcut(t, server, "edit", map[string]string{"id": "42"}) + if err == nil { + t.Fatal("expected error when no edit fields are provided") + } +} + +func TestPREditHTTPError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("server error")) + })) + defer server.Close() + + err := runPRShortcut(t, server, "edit", map[string]string{"id": "42", "title": "x"}) + if err == nil { + t.Fatal("expected error for HTTP 500") + } +} + // --- extractIssueID --- func TestExtractIssueID(t *testing.T) {