Add support for `gh pr merge` by probablycorey · Pull Request #873 · cli/cli

probablycorey · 2020-05-06T18:26:57Z

This adds the gh pr merge command and it works like this.

gh pr merge merges the current PR
gh pr merge 54 merges PR #54

It has three mutually exclusive flags --rebase, --merge, and --squash. The default flag is --merge

Here is what it looks like:

What will be in follow up PRs

Making gh pr merge [number | url] interactive if there are no flags
Investigate how much work it is to follow the merge rules setup for a branch

Part 1 of closing #373

probablycorey · 2020-05-06T18:49:38Z

+	initWithStubs("master",
+		stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": {
+			"pullRequest": { "number": 1, "closed": false, "state": "OPEN"}
+		} } }`)},
+		stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)},
+	)


I'm wondering what people think about this. Creating the blank context and the fake http instance seemed like it was getting very repetitive in tests with not much added benefit. Are helpers like this helpful?

Indeed, it's getting really repetitive, especially because we are forced to copy-paste JSON response stubs for API calls that are not even being exercised in the current test. I think that initWithStubs, but not that we might be moving away from sequential stubs to those that match specific requests based on their contents #874 which means we might need access to additional apis other than just http.StubResponse

probablycorey · 2020-05-06T18:51:32Z

 }

+var prMergeCmd = &cobra.Command{
+	Use:   "merge [number | url] [--rebase | --merge | --squash]",


I wasn't sure how to document the idea of a mutually exclusive optional variable.
[{number | url}] seems like it might be correct, but [number | url] is easier to read and still seems unambiguous to me. It's hard to tell from command line docs what the correct syntax is.

The 1st argument is the same as the optional argument to view and checkout, right? So it would be

[{<number> | <url> | <branch>}]

what follows are flags that are optional & mutually exclusive:

[{--rebase | --merge | --squash}]

however, if all this is getting unwieldy, I don't think we strictly need to document these flags in command synopsis line. Users will read about the available flags in the FLAGS section.

mislav · 2020-05-07T14:39:21Z

 }

+var prMergeCmd = &cobra.Command{
+	Use:   "merge [number | url] [--rebase | --merge | --squash]",


The 1st argument is the same as the optional argument to view and checkout, right? So it would be

[{<number> | <url> | <branch>}]

what follows are flags that are optional & mutually exclusive:

[{--rebase | --merge | --squash}]

however, if all this is getting unwieldy, I don't think we strictly need to document these flags in command synopsis line. Users will read about the available flags in the FLAGS section.

mislav · 2020-05-07T15:02:54Z

+	initWithStubs("master",
+		stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": {
+			"pullRequest": { "number": 1, "closed": false, "state": "OPEN"}
+		} } }`)},
+		stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)},
+	)


Indeed, it's getting really repetitive, especially because we are forced to copy-paste JSON response stubs for API calls that are not even being exercised in the current test. I think that initWithStubs, but not that we might be moving away from sequential stubs to those that match specific requests based on their contents #874 which means we might need access to additional apis other than just http.StubResponse

mislav · 2020-05-07T15:04:26Z

+
+func merge(client *Client, repo ghrepo.Interface, pr *PullRequest, mergeMethod githubv4.PullRequestMergeMethod) error {
+	var mutation struct {
+		ReopenPullRequest struct {


nit: copy-pasta

A little scary that everything worked fine with that error!

Ah, well, you can name your struct fields anything you like, and the GraphQL library will either request a field/connection/mutation based on that field name or based on the graphql:"..." struct tag. Since this field has a tag graphql:"mergePullRequest(input: $input)", the mutation that is requested is always mergePullRequest and technically your overall query is correct regardless of the struct field name you choose.

probablycorey · 2020-05-07T17:52:42Z

This should be good to go now.

mislav

Looks great! Thank you

mislav · 2020-05-07T19:45:30Z

 }

+var prMergeCmd = &cobra.Command{
+	Use:   "merge [{<number> | <url>}] [<flags>]",


PR commands generally also accept <branch> value as alternative to <number> and <url>, so it should be listed here for consistency

Should we ever note [<flags>] explicitly? It used to be added automatically for us by Cobra output for any command that supports flags, but not anymore it seems due to Help output for individual commands is broken #883

I guess we don't need to add flag to the usage string. I'll add branch and get rid of flag.

mislav · 2020-05-08T10:21:46Z

+	return merge(client, repo, pr, githubv4.PullRequestMergeMethodRebase)
+}
+
+func PullRequestSquash(client *Client, repo ghrepo.Interface, pr *PullRequest) error {


Instead of having a separate method for each of the merge options, could we pass a constant as an argument to a single method called e.g. PullRequestMerge()? We'd define 3 constants in the api package and then inside the method we would translate them to appropriate githubv4 constants. This is the approach @vilmibm took in pr review and I dig it!

I originally tried something like this but ended up having lots of type conversion issues. I'll try this switch statement idea

Ahhh... this worked great. Thanks for the example link.

probablycorey · 2020-05-08T18:42:20Z

This is ready to go. But I'm going to hold off on merging it until after we release on monday

TomasVotruba · 2020-05-27T21:15:34Z

I missed this for so long! Thank you for adding it 👍

probablycorey added 7 commits May 5, 2020 11:35

Add simple merge test

16ad382

Add merge code

0aca0ef

Add merge api call

c0831d4

Make squash and rebase work

8681e7a

Add 'already merged' test

062c835

Make it work without a PR number

8c85e14

Remove debug output

5a04679

probablycorey requested review from mislav and vilmibm May 6, 2020 18:26

probablycorey self-assigned this May 6, 2020

probablycorey added 2 commits May 6, 2020 11:27

Fix lint error

1de57db

update test

3d21a33

probablycorey commented May 6, 2020

View reviewed changes

mislav suggested changes May 7, 2020

View reviewed changes

probablycorey added 8 commits May 7, 2020 10:29

Use correct struct

5810acf

Fix sentence case

bcf41fd

Fix usage syntax

2041f0a

Fix merge usage syntax

1ea38af

Use PR if given

4b2f14d

Use stdout

5b78d47

Better description

58663dc

Update pr_test.go

ea6b3dc

mislav approved these changes May 8, 2020

View reviewed changes

probablycorey added 2 commits May 8, 2020 11:20

Update usage docs

b9c4a76

Use a var

43e1513

billygriffin mentioned this pull request May 8, 2020

Support to merge pull request from gh cli #373

Closed

jetersen mentioned this pull request May 11, 2020

Implement PR merge command mislav/hub#2280

Merged

mislav approved these changes May 11, 2020

View reviewed changes

probablycorey merged commit ad3a590 into master May 12, 2020

mislav deleted the the-merge-dubai branch June 25, 2020 14:37

coderabbitai Bot mentioned this pull request May 8, 2026

Fix backend test scripts to run actual test suite via pytest strikersam/autonomous-ai-agency#93

Merged

Conversation

probablycorey commented May 6, 2020 • edited by billygriffin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

probablycorey commented May 7, 2020

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

probablycorey commented May 8, 2020

Uh oh!

TomasVotruba commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

probablycorey commented May 6, 2020 •

edited by billygriffin

Loading

TomasVotruba commented May 27, 2020 •

edited

Loading