Skip to content

Commit d3dd8be

Browse files
nvandesselclaude
andcommitted
fix: address code review findings
- Pass opts.Body as --description to gt submit (was silently dropped) - Return created/updated status from parseSubmitResult instead of hardcoding Created: true (uses the captured group from gt output) - Add PR state constants to driver package, replace magic string "MERGED" in sync.go (reverts regression from PR #20's fix) - Add NewNative() constructor with gh.Available() check, matching the pattern established by NewGraphite() for gt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 97e88c4 commit d3dd8be

File tree

6 files changed

+69
-32
lines changed

6 files changed

+69
-32
lines changed

cmd/sync.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ func runSync(cmd *cobra.Command, args []string) error {
100100
fmt.Fprintf(os.Stderr, "warning: could not check PR #%d for %s: %v\n", *b.PR, name, err)
101101
continue
102102
}
103-
if prState == "MERGED" {
103+
if prState == driver.PRStateMerged {
104104
mergedBranches = append(mergedBranches, name)
105105
mergedData[name] = b
106106
}

internal/driver/driver.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,12 +56,19 @@ func (e *RebaseConflictError) Error() string {
5656
return fmt.Sprintf("rebase conflict on branch %s: %s", e.Branch, e.Detail)
5757
}
5858

59+
// PR state constants returned by PRState.
60+
const (
61+
PRStateOpen = "OPEN"
62+
PRStateClosed = "CLOSED"
63+
PRStateMerged = "MERGED"
64+
)
65+
5966
// Resolve returns the Driver for the given driver name.
6067
// An empty name resolves to the native (git+gh) driver.
6168
func Resolve(name string) (Driver, error) {
6269
switch name {
6370
case "", "native":
64-
return &Native{}, nil
71+
return NewNative()
6572
case "graphite":
6673
return NewGraphite()
6774
default:

internal/driver/driver_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,17 @@ import (
77
)
88

99
func TestResolveNative(t *testing.T) {
10+
_, err := exec.LookPath("gh")
11+
if err != nil {
12+
// gh not installed — Resolve should fail with a descriptive error.
13+
_, resolveErr := Resolve("")
14+
if resolveErr == nil {
15+
t.Fatal("Resolve('') should fail when gh is not installed")
16+
}
17+
t.Logf("gh not installed, Resolve error: %v (expected)", resolveErr)
18+
return
19+
}
20+
1021
drv, err := Resolve("")
1122
if err != nil {
1223
t.Fatalf("Resolve empty: %v", err)

internal/driver/graphite.go

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ func (g *Graphite) Push(ctx context.Context, opts PushOpts) (*PushResult, error)
5050
if opts.Title != "" && opts.ExistingPR == nil {
5151
args = append(args, "--title", opts.Title)
5252
}
53+
if opts.Body != "" && opts.ExistingPR == nil {
54+
args = append(args, "--description", opts.Body)
55+
}
5356

5457
out, err := runGT(ctx, args...)
5558
if err != nil {
@@ -61,12 +64,12 @@ func (g *Graphite) Push(ctx context.Context, opts PushOpts) (*PushResult, error)
6164
return &PushResult{PRNumber: *opts.ExistingPR, Created: false}, nil
6265
}
6366

64-
// Parse PR number from gt submit output.
65-
prNum, err := parsePRNumber(out, opts.Branch)
67+
// Parse PR number and created/updated status from gt submit output.
68+
prNum, created, err := parseSubmitResult(out, opts.Branch)
6669
if err != nil {
6770
return nil, fmt.Errorf("parsing PR number from gt submit output: %w", err)
6871
}
69-
return &PushResult{PRNumber: prNum, Created: true}, nil
72+
return &PushResult{PRNumber: prNum, Created: created}, nil
7073
}
7174

7275
func (g *Graphite) Rebase(ctx context.Context, _, _ string) error {
@@ -95,9 +98,10 @@ func runGT(ctx context.Context, args ...string) (string, error) {
9598
return strings.TrimSpace(out.String()), err
9699
}
97100

98-
// parsePRNumber extracts the PR number for branch from gt submit output.
101+
// parseSubmitResult extracts the PR number and created/updated status for
102+
// branch from gt submit output.
99103
// gt submit prints one line per branch: "<branch>: <url> (created|updated)"
100-
func parsePRNumber(output, branch string) (int, error) {
104+
func parseSubmitResult(output, branch string) (prNumber int, created bool, err error) {
101105
for _, line := range strings.Split(output, "\n") {
102106
line = strings.TrimSpace(line)
103107
matches := submitLineRe.FindStringSubmatch(line)
@@ -110,13 +114,13 @@ func parsePRNumber(output, branch string) (int, error) {
110114
url := matches[2]
111115
idx := strings.LastIndex(url, "/")
112116
if idx == -1 || idx == len(url)-1 {
113-
return 0, fmt.Errorf("malformed PR URL %q: no trailing number", url)
117+
return 0, false, fmt.Errorf("malformed PR URL %q: no trailing number", url)
114118
}
115-
num, err := strconv.Atoi(url[idx+1:])
116-
if err != nil {
117-
return 0, fmt.Errorf("malformed PR URL %q: %w", url, err)
119+
num, parseErr := strconv.Atoi(url[idx+1:])
120+
if parseErr != nil {
121+
return 0, false, fmt.Errorf("malformed PR URL %q: %w", url, parseErr)
118122
}
119-
return num, nil
123+
return num, matches[3] == "created", nil
120124
}
121-
return 0, fmt.Errorf("branch %q not found in gt submit output:\n%s", branch, output)
125+
return 0, false, fmt.Errorf("branch %q not found in gt submit output:\n%s", branch, output)
122126
}

internal/driver/graphite_test.go

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,33 +5,37 @@ import (
55
"testing"
66
)
77

8-
func TestParsePRNumber(t *testing.T) {
8+
func TestParseSubmitResult(t *testing.T) {
99
tests := []struct {
10-
name string
11-
output string
12-
branch string
13-
want int
14-
wantErr string
10+
name string
11+
output string
12+
branch string
13+
wantPR int
14+
wantCreated bool
15+
wantErr string
1516
}{
1617
{
17-
name: "created on .com domain",
18-
output: "my-feature: https://app.graphite.com/github/pr/owner/repo/42 (created)",
19-
branch: "my-feature",
20-
want: 42,
18+
name: "created on .com domain",
19+
output: "my-feature: https://app.graphite.com/github/pr/owner/repo/42 (created)",
20+
branch: "my-feature",
21+
wantPR: 42,
22+
wantCreated: true,
2123
},
2224
{
23-
name: "updated on .dev domain",
24-
output: "my-feature: https://app.graphite.dev/github/pr/owner/repo/99 (updated)",
25-
branch: "my-feature",
26-
want: 99,
25+
name: "updated on .dev domain",
26+
output: "my-feature: https://app.graphite.dev/github/pr/owner/repo/99 (updated)",
27+
branch: "my-feature",
28+
wantPR: 99,
29+
wantCreated: false,
2730
},
2831
{
2932
name: "multi-branch stack matches correct branch",
3033
output: `pp--06-14-part_1: https://app.graphite.com/github/pr/withgraphite/repo/100 (created)
3134
pp--06-14-part_2: https://app.graphite.com/github/pr/withgraphite/repo/101 (created)
3235
pp--06-14-part_3: https://app.graphite.com/github/pr/withgraphite/repo/102 (created)`,
33-
branch: "pp--06-14-part_2",
34-
want: 101,
36+
branch: "pp--06-14-part_2",
37+
wantPR: 101,
38+
wantCreated: true,
3539
},
3640
{
3741
name: "branch not found",
@@ -55,7 +59,7 @@ pp--06-14-part_3: https://app.graphite.com/github/pr/withgraphite/repo/102 (crea
5559

5660
for _, tt := range tests {
5761
t.Run(tt.name, func(t *testing.T) {
58-
got, err := parsePRNumber(tt.output, tt.branch)
62+
gotPR, gotCreated, err := parseSubmitResult(tt.output, tt.branch)
5963
if tt.wantErr != "" {
6064
if err == nil {
6165
t.Fatalf("expected error containing %q, got nil", tt.wantErr)
@@ -68,8 +72,11 @@ pp--06-14-part_3: https://app.graphite.com/github/pr/withgraphite/repo/102 (crea
6872
if err != nil {
6973
t.Fatalf("unexpected error: %v", err)
7074
}
71-
if got != tt.want {
72-
t.Errorf("parsePRNumber() = %d, want %d", got, tt.want)
75+
if gotPR != tt.wantPR {
76+
t.Errorf("prNumber = %d, want %d", gotPR, tt.wantPR)
77+
}
78+
if gotCreated != tt.wantCreated {
79+
t.Errorf("created = %v, want %v", gotCreated, tt.wantCreated)
7380
}
7481
})
7582
}

internal/driver/native.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,14 @@ import (
1212
// Native is the default driver using git + gh CLIs directly.
1313
type Native struct{}
1414

15+
// NewNative validates that gh is installed and returns a Native driver.
16+
func NewNative() (*Native, error) {
17+
if err := gh.Available(); err != nil {
18+
return nil, err
19+
}
20+
return &Native{}, nil
21+
}
22+
1523
func (n *Native) Name() string { return "native" }
1624

1725
func (n *Native) CurrentBranch(ctx context.Context) (string, error) {

0 commit comments

Comments
 (0)