pulls: Support updating base branch by abhinav · Pull Request #528 · google/go-github

abhinav · 2017-01-22T07:51:09Z

This adds support for changing the base branch of a PR.

As per the discussion in #421, the PullRequest is converted into an
unexported type pullRequestUpdate which matches the shape expected by the
pulls PATCH endpoint.

Resolves issue #421.

This adds support for changing the base branch of a PR. As per the discussion in #421, the `PullRequest` is converted into an unexported type `pullRequestUpdate` which matches the shape expected by the pulls PATCH endpoint. Issue: #421

dmitshur

Thanks for making this PR @abhinav, this looks really good. I think we need to improve documentation, then this can be merged. See other comments about an optional enhancement to the tests.

dmitshur · 2017-01-26T22:12:41Z

github/pulls.go

+
 // Edit a pull request.
 //
 // GitHub API docs: https://developer.github.com/v3/pulls/#update-a-pull-request


I think we should document which fields we currently support. It's not obvious that passing github.&PullRequest{Base: &github.PullRequestBranch{Ref: "belong-to-us"}, out out of all other fields inside PullRequest struct, will update the base.

Something like:

// Edit a pull request. // // Fields that can be edited are: Title, Body, State, Base.Ref to edit base branch.

Agreed. Added docs for this.

dmitshur · 2017-01-26T23:58:49Z

github/pulls_test.go

+		input        *PullRequest
+		sendResponse string
+
+		wantUpdate   *pullRequestUpdate


This is optional, but I think it'd be better if you made the type of wantUpdate a string and have it contain the expected JSON.

There's no need to decode the update JSON into our own struct that we used for encoding, and compare those.

What do you think?

I was concerned that the order in which the struct fields would
be serialized is not deterministic but that doesn't appear to be
the case upon trial. Updated.

I see.

Yeah, encoding/json encoder uses the order of fields in a struct when encoding, so it is deterministic. It even sorts map keys, so everything's always deterministic.

dmitshur · 2017-01-26T23:59:46Z

github/pulls_test.go

+		{
+			input:        &PullRequest{Title: String("t")},
+			sendResponse: `{"number":1}`,
+			wantUpdate:   &pullRequestUpdate{Title: String("t")},


Then, the value for wantUpdate here would be something like {"title":"t"}.

It's easier to reason about it being correct, since GitHub API deals with JSON input and we can compare its documentation with this.

dmitshur

LGTM. Thank you!

See line comment for another simplification opportunity.

dmitshur · 2017-01-28T23:27:19Z

github/pulls_test.go

+				wantUpdate := tt.wantUpdate + "\n" // json encoder adds a newline
+				if string(gotUpdate) != wantUpdate {
+					t.Errorf("Request body = %q, want %q", gotUpdate, wantUpdate)
 				}


You can use testBody helper to simplify this.

See

go-github/github/pulls_test.go

Line 478 in c9c37fd

testBody(t, r, test.wantBody+"\n")

for example.

dmitshur · 2017-01-28T23:32:07Z

github/pulls_test.go

+	for _, tt := range tests {
+		func() {
+			setup()
+			defer teardown()


In fact, you can use the idea from TestPullRequestsService_Merge_options to remove the need for the nested closure with per-testcase setup()/defer teardown(). The idea there is to use i as the pull request ID, so that registered routes don't overlap.

It also tests that the handler gets called (instead of not being called).

See

go-github/github/pulls_test.go

Lines 438 to 486 in c9c37fd

// Test that different merge options produce expected PUT requests. See issue https://github.com/google/go-github/issues/500.

func TestPullRequestsService_Merge_options(t *testing.T) {

setup()

defer teardown()

tests := []struct {

options *PullRequestOptions

wantBody string

}{

{

options: nil,

wantBody: `{"commit_message":"merging pull request"}`,

},

{

options: &PullRequestOptions{},

wantBody: `{"commit_message":"merging pull request"}`,

},

{

options: &PullRequestOptions{MergeMethod: "rebase"},

wantBody: `{"commit_message":"merging pull request","merge_method":"rebase"}`,

},

{

options: &PullRequestOptions{SHA: "6dcb09b5b57875f334f61aebed695e2e4193db5e"},

wantBody: `{"commit_message":"merging pull request","sha":"6dcb09b5b57875f334f61aebed695e2e4193db5e"}`,

},

{

options: &PullRequestOptions{

CommitTitle: "Extra detail",

SHA: "6dcb09b5b57875f334f61aebed695e2e4193db5e",

MergeMethod: "squash",

},

wantBody: `{"commit_message":"merging pull request","commit_title":"Extra detail","merge_method":"squash","sha":"6dcb09b5b57875f334f61aebed695e2e4193db5e"}`,

},

}

for i, test := range tests {

madeRequest := false

mux.HandleFunc(fmt.Sprintf("/repos/o/r/pulls/%d/merge", i), func(w http.ResponseWriter, r *http.Request) {

testMethod(t, r, "PUT")

testHeader(t, r, "Accept", mediaTypeSquashPreview)

testBody(t, r, test.wantBody+"\n")

madeRequest = true

})

_, _, _ = client.PullRequests.Merge("o", "r", i, "merging pull request", test.options)

if !madeRequest {

t.Errorf("%d: PullRequests.Merge(%#v): expected request was not made", i, test.options)

}

}

}

.

Optional, if you're interested in doing this.

Also test whether the request was acually made.

dmitshur

Looks great, thanks for doing this!

LGTM.

gmlewis

LGTM (apart from a tiny nit).
Thank you @abhinav and @shurcooL!

gmlewis · 2017-01-29T21:32:37Z

github/pulls_test.go

-		v := new(PullRequest)
-		json.NewDecoder(r.Body).Decode(v)
+		wantUpdate   string
+		wantResponse *PullRequest


nit: I find wantResponse a bit confusing here... what if we just called it want ?
Either way is fine, though... your call.

As per the discussion in google#421, the PullRequest parameter is converted into an unexported type pullRequestUpdate which matches the shape expected by the pulls PATCH endpoint. Resolves google#421.

pulls: Support updating base branch

a94b581

This adds support for changing the base branch of a PR. As per the discussion in #421, the `PullRequest` is converted into an unexported type `pullRequestUpdate` which matches the shape expected by the pulls PATCH endpoint. Issue: #421

dmitshur mentioned this pull request Jan 26, 2017

What happens if you Git.CreateCommit(..., nil) or PullRequests.Edit(..., nil)? #531

Closed

dmitshur requested changes Jan 27, 2017

View reviewed changes

abhinav added 2 commits January 27, 2017 22:12

pulls: Document Edit() better

8304959

pulls: Simplify Edit test

50056f3

dmitshur approved these changes Jan 28, 2017

View reviewed changes

dmitshur reviewed Jan 28, 2017

View reviewed changes

pulls: Simplify test further

4651fa7

Also test whether the request was acually made.

dmitshur approved these changes Jan 29, 2017

View reviewed changes

gmlewis approved these changes Jan 29, 2017

View reviewed changes

s/wantResponse/want/

4ee5d1d

dmitshur mentioned this pull request Jan 31, 2017

Policy on using pointers/values in structs for receiving data from GitHub servers. #537

Closed

dmitshur self-assigned this Feb 1, 2017

dmitshur merged commit 1d8328d into google:master Feb 1, 2017

abhinav deleted the pull-request-update-base branch February 8, 2017 07:20

	// Test that different merge options produce expected PUT requests. See issue https://github.com/google/go-github/issues/500.
	func TestPullRequestsService_Merge_options(t *testing.T) {
	setup()
	defer teardown()

	tests := []struct {
	options *PullRequestOptions
	wantBody string
	}{
	{
	options: nil,
	wantBody: `{"commit_message":"merging pull request"}`,
	},
	{
	options: &PullRequestOptions{},
	wantBody: `{"commit_message":"merging pull request"}`,
	},
	{
	options: &PullRequestOptions{MergeMethod: "rebase"},
	wantBody: `{"commit_message":"merging pull request","merge_method":"rebase"}`,
	},
	{
	options: &PullRequestOptions{SHA: "6dcb09b5b57875f334f61aebed695e2e4193db5e"},
	wantBody: `{"commit_message":"merging pull request","sha":"6dcb09b5b57875f334f61aebed695e2e4193db5e"}`,
	},
	{
	options: &PullRequestOptions{
	CommitTitle: "Extra detail",
	SHA: "6dcb09b5b57875f334f61aebed695e2e4193db5e",
	MergeMethod: "squash",
	},
	wantBody: `{"commit_message":"merging pull request","commit_title":"Extra detail","merge_method":"squash","sha":"6dcb09b5b57875f334f61aebed695e2e4193db5e"}`,
	},
	}

	for i, test := range tests {
	madeRequest := false
	mux.HandleFunc(fmt.Sprintf("/repos/o/r/pulls/%d/merge", i), func(w http.ResponseWriter, r *http.Request) {
	testMethod(t, r, "PUT")
	testHeader(t, r, "Accept", mediaTypeSquashPreview)
	testBody(t, r, test.wantBody+"\n")
	madeRequest = true
	})
	_, _, _ = client.PullRequests.Merge("o", "r", i, "merging pull request", test.options)
	if !madeRequest {
	t.Errorf("%d: PullRequests.Merge(%#v): expected request was not made", i, test.options)
	}
	}
	}

Conversation

abhinav commented Jan 22, 2017 • edited by dmitshur Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitshur left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Jan 26, 2017

Choose a reason for hiding this comment

Uh oh!

abhinav Jan 28, 2017

Choose a reason for hiding this comment

Uh oh!

dmitshur Jan 26, 2017

Choose a reason for hiding this comment

Uh oh!

abhinav Jan 28, 2017

Choose a reason for hiding this comment

Uh oh!

dmitshur Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Jan 26, 2017

Choose a reason for hiding this comment

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

dmitshur Jan 28, 2017

Choose a reason for hiding this comment

Uh oh!

dmitshur Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis Jan 29, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abhinav commented Jan 22, 2017 •

edited by dmitshur

Loading

dmitshur left a comment •

edited

Loading

dmitshur Jan 28, 2017 •

edited

Loading

dmitshur Jan 28, 2017 •

edited

Loading