From db93444ba998e3bec1311ad4efd1042abc2800cf Mon Sep 17 00:00:00 2001 From: nobe4 Date: Fri, 1 Mar 2024 13:26:20 +0100 Subject: [PATCH] feat(merge): add owner/repo in prompt Various places during the `gh pr merge` flow show the PR number and title. Those places are updated to also show the owner/repo. E.g. Before: Pull request #123 (title) is ready to be merged After: Pull request owner/repo#123 (title) is ready to be merged There are other places, where only the number is displayed. Those were intentionally left as is. It made sense to show the owner/repo only when the extra context of the title was present. It also fixes the related tests. cc #8777 --- pkg/cmd/pr/merge/merge.go | 6 ++--- pkg/cmd/pr/merge/merge_test.go | 48 +++++++++++++++++----------------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 4a3db447d..e802c0cd0 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -235,7 +235,7 @@ func (m *mergeContext) warnIfDiverged() { return } - _ = m.warnf("%s Pull request #%d (%s) has diverged from local branch\n", m.cs.Yellow("!"), m.pr.Number, m.pr.Title) + _ = m.warnf("%s Pull request %s/%s#%d (%s) has diverged from local branch\n", m.cs.Yellow("!"), m.baseRepo.RepoOwner(), m.baseRepo.RepoName(), m.pr.Number, m.pr.Title) } // Check if the current state of the pull request allows for merging @@ -302,7 +302,7 @@ func (m *mergeContext) merge() error { return cmdutil.FlagErrorf("--merge, --rebase, or --squash required when not running interactively") } - _ = m.infof("Merging pull request #%d (%s)\n", m.pr.Number, m.pr.Title) + _ = m.infof("Merging pull request %s/%s#%d (%s)\n", m.baseRepo.RepoOwner(), m.baseRepo.RepoName(), m.pr.Number, m.pr.Title) apiClient := api.NewClientFromHTTP(m.httpClient) r, err := api.GitHubRepo(apiClient, m.baseRepo) @@ -366,7 +366,7 @@ func (m *mergeContext) merge() error { case PullRequestMergeMethodSquash: action = "Squashed and merged" } - return m.infof("%s %s pull request #%d (%s)\n", m.cs.SuccessIconWithColor(m.cs.Magenta), action, m.pr.Number, m.pr.Title) + return m.infof("%s %s pull request %s/%s#%d (%s)\n", m.cs.SuccessIconWithColor(m.cs.Magenta), action, m.baseRepo.RepoOwner(), m.baseRepo.RepoName(), m.pr.Number, m.pr.Title) } // Delete local branch if requested and if allowed. diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 0eb6a81fb..8561e3951 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -337,7 +337,7 @@ func TestPrMerge(t *testing.T) { t.Fatalf("error running command `pr merge`: %v", err) } - r := regexp.MustCompile(`Merged pull request #1 \(The title of the PR\)`) + r := regexp.MustCompile(`Merged pull request OWNER/REPO#1 \(The title of the PR\)`) if !r.MatchString(output.Stderr()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) @@ -518,7 +518,7 @@ func TestPrMerge_withRepoFlag(t *testing.T) { t.Fatalf("error running command `pr merge`: %v", err) } - r := regexp.MustCompile(`Merged pull request #1 \(The title of the PR\)`) + r := regexp.MustCompile(`Merged pull request OWNER/REPO#1 \(The title of the PR\)`) if !r.MatchString(output.Stderr()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) @@ -559,7 +559,7 @@ func TestPrMerge_withMatchCommitHeadFlag(t *testing.T) { t.Fatalf("error running command `pr merge`: %v", err) } - r := regexp.MustCompile(`Merged pull request #1 \(The title of the PR\)`) + r := regexp.MustCompile(`Merged pull request OWNER/REPO#1 \(The title of the PR\)`) if !r.MatchString(output.Stderr()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) @@ -601,7 +601,7 @@ func TestPrMerge_withAuthorFlag(t *testing.T) { t.Fatalf("error running command `pr merge`: %v", err) } - r := regexp.MustCompile(`Merged pull request #1 \(The title of the PR\)`) + r := regexp.MustCompile(`Merged pull request OWNER/REPO#1 \(The title of the PR\)`) if !r.MatchString(output.Stderr()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) @@ -653,7 +653,7 @@ func TestPrMerge_deleteBranch(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, heredoc.Doc(` - ✓ Merged pull request #10 (Blueberries are a good fruit) + ✓ Merged pull request OWNER/REPO#10 (Blueberries are a good fruit) ✓ Deleted local branch blueberries and switched to branch main ✓ Deleted remote branch blueberries `), output.Stderr()) @@ -704,7 +704,7 @@ func TestPrMerge_deleteBranch_nonDefault(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, heredoc.Doc(` - ✓ Merged pull request #10 (Blueberries are a good fruit) + ✓ Merged pull request OWNER/REPO#10 (Blueberries are a good fruit) ✓ Deleted local branch blueberries and switched to branch fruit ✓ Deleted remote branch blueberries `), output.Stderr()) @@ -753,7 +753,7 @@ func TestPrMerge_deleteBranch_onlyLocally(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, heredoc.Doc(` - ✓ Merged pull request #10 (Blueberries are a good fruit) + ✓ Merged pull request OWNER/REPO#10 (Blueberries are a good fruit) ✓ Deleted local branch blueberries and switched to branch main `), output.Stderr()) } @@ -803,7 +803,7 @@ func TestPrMerge_deleteBranch_checkoutNewBranch(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, heredoc.Doc(` - ✓ Merged pull request #10 (Blueberries are a good fruit) + ✓ Merged pull request OWNER/REPO#10 (Blueberries are a good fruit) ✓ Deleted local branch blueberries and switched to branch fruit ✓ Deleted remote branch blueberries `), output.Stderr()) @@ -850,7 +850,7 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, heredoc.Doc(` - ✓ Merged pull request #10 (Blueberries are a good fruit) + ✓ Merged pull request OWNER/REPO#10 (Blueberries are a good fruit) ✓ Deleted local branch blueberries ✓ Deleted remote branch blueberries `), output.Stderr()) @@ -892,7 +892,7 @@ func Test_nonDivergingPullRequest(t *testing.T) { } assert.Equal(t, heredoc.Doc(` - ✓ Merged pull request #10 (Blueberries are a good fruit) + ✓ Merged pull request OWNER/REPO#10 (Blueberries are a good fruit) `), output.Stderr()) } @@ -932,8 +932,8 @@ func Test_divergingPullRequestWarning(t *testing.T) { } assert.Equal(t, heredoc.Doc(` - ! Pull request #10 (Blueberries are a good fruit) has diverged from local branch - ✓ Merged pull request #10 (Blueberries are a good fruit) + ! Pull request OWNER/REPO#10 (Blueberries are a good fruit) has diverged from local branch + ✓ Merged pull request OWNER/REPO#10 (Blueberries are a good fruit) `), output.Stderr()) } @@ -972,7 +972,7 @@ func Test_pullRequestWithoutCommits(t *testing.T) { } assert.Equal(t, heredoc.Doc(` - ✓ Merged pull request #10 (Blueberries are a good fruit) + ✓ Merged pull request OWNER/REPO#10 (Blueberries are a good fruit) `), output.Stderr()) } @@ -1010,7 +1010,7 @@ func TestPrMerge_rebase(t *testing.T) { t.Fatalf("error running command `pr merge`: %v", err) } - r := regexp.MustCompile(`Rebased and merged pull request #2 \(The title of the PR\)`) + r := regexp.MustCompile(`Rebased and merged pull request OWNER/REPO#2 \(The title of the PR\)`) if !r.MatchString(output.Stderr()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) @@ -1053,7 +1053,7 @@ func TestPrMerge_squash(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, heredoc.Doc(` - ✓ Squashed and merged pull request #3 (The title of the PR) + ✓ Squashed and merged pull request OWNER/REPO#3 (The title of the PR) `), output.Stderr()) } @@ -1275,7 +1275,7 @@ func TestPRMergeTTY(t *testing.T) { t.Fatalf("Got unexpected error running `pr merge` %s", err) } - assert.Equal(t, "Merging pull request #3 (It was the best of times)\n✓ Merged pull request #3 (It was the best of times)\n", output.Stderr()) + assert.Equal(t, "Merging pull request OWNER/REPO#3 (It was the best of times)\n✓ Merged pull request OWNER/REPO#3 (It was the best of times)\n", output.Stderr()) } func TestPRMergeTTY_withDeleteBranch(t *testing.T) { @@ -1346,8 +1346,8 @@ func TestPRMergeTTY_withDeleteBranch(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, heredoc.Doc(` - Merging pull request #3 (It was the best of times) - ✓ Merged pull request #3 (It was the best of times) + Merging pull request OWNER/REPO#3 (It was the best of times) + ✓ Merged pull request OWNER/REPO#3 (It was the best of times) ✓ Deleted local branch blueberries and switched to branch main ✓ Deleted remote branch blueberries `), output.Stderr()) @@ -1438,7 +1438,7 @@ func TestPRMergeTTY_squashEditCommitMsgAndSubject(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "", stdout.String()) - assert.Equal(t, "Merging pull request #123 (title)\n✓ Squashed and merged pull request #123 (title)\n", stderr.String()) + assert.Equal(t, "Merging pull request OWNER/REPO#123 (title)\n✓ Squashed and merged pull request OWNER/REPO#123 (title)\n", stderr.String()) } func TestPRMergeEmptyStrategyNonTTY(t *testing.T) { @@ -1517,7 +1517,7 @@ func TestPRTTY_cancelled(t *testing.T) { t.Fatalf("got error %v", err) } - assert.Equal(t, "Merging pull request #123 (title)\nCancelled.\n", output.Stderr()) + assert.Equal(t, "Merging pull request OWNER/REPO#123 (title)\nCancelled.\n", output.Stderr()) } func Test_mergeMethodSurvey(t *testing.T) { @@ -1576,7 +1576,7 @@ func TestMergeRun_autoMerge(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "", stdout.String()) - assert.Equal(t, "✓ Pull request #123 will be automatically merged via squash when all requirements are met\n", stderr.String()) + assert.Equal(t, "✓ Pull request #123 will be automatically merged via squash when all requirements are met\n", stderr.String()) } func TestMergeRun_autoMerge_directMerge(t *testing.T) { @@ -1614,7 +1614,7 @@ func TestMergeRun_autoMerge_directMerge(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "", stdout.String()) - assert.Equal(t, "✓ Merged pull request #123 ()\n", stderr.String()) + assert.Equal(t, "✓ Merged pull request OWNER/REPO#123 ()\n", stderr.String()) } func TestMergeRun_disableAutoMerge(t *testing.T) { @@ -1867,7 +1867,7 @@ func TestPrAddToMergeQueueAdmin(t *testing.T) { } assert.Equal(t, "", output.String()) - assert.Equal(t, "Merging pull request #1 (The title of the PR)\n✓ Merged pull request #1 (The title of the PR)\n", output.Stderr()) + assert.Equal(t, "Merging pull request OWNER/REPO#1 (The title of the PR)\n✓ Merged pull request OWNER/REPO#1 (The title of the PR)\n", output.Stderr()) } func TestPrAddToMergeQueueAdminWithMergeStrategy(t *testing.T) { @@ -1906,7 +1906,7 @@ func TestPrAddToMergeQueueAdminWithMergeStrategy(t *testing.T) { } assert.Equal(t, "", output.String()) - assert.Equal(t, "✓ Merged pull request #1 (The title of the PR)\n", output.Stderr()) + assert.Equal(t, "✓ Merged pull request OWNER/REPO#1 (The title of the PR)\n", output.Stderr()) } type testEditor struct{}