From a94b58174259b38e70d1bc9776b47227fbed9847 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sat, 21 Jan 2017 23:44:22 -0800 Subject: [PATCH 1/5] pulls: Support updating base branch 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 --- github/pulls.go | 20 ++++++++++++- github/pulls_test.go | 71 ++++++++++++++++++++++++++++++++------------ 2 files changed, 71 insertions(+), 20 deletions(-) diff --git a/github/pulls.go b/github/pulls.go index 51c6ccb576c..c3dc2b73e89 100644 --- a/github/pulls.go +++ b/github/pulls.go @@ -189,12 +189,30 @@ func (s *PullRequestsService) Create(owner string, repo string, pull *NewPullReq return p, resp, err } +type pullRequestUpdate struct { + Title *string `json:"title,omitempty"` + Body *string `json:"body,omitempty"` + State *string `json:"state,omitempty"` + Base *string `json:"base,omitempty"` +} + // Edit a pull request. // // GitHub API docs: https://developer.github.com/v3/pulls/#update-a-pull-request func (s *PullRequestsService) Edit(owner string, repo string, number int, pull *PullRequest) (*PullRequest, *Response, error) { u := fmt.Sprintf("repos/%v/%v/pulls/%d", owner, repo, number) - req, err := s.client.NewRequest("PATCH", u, pull) + + update := new(pullRequestUpdate) + if pull != nil { + update.Title = pull.Title + update.Body = pull.Body + update.State = pull.State + if pull.Base != nil { + update.Base = pull.Base.Ref + } + } + + req, err := s.client.NewRequest("PATCH", u, update) if err != nil { return nil, nil, err } diff --git a/github/pulls_test.go b/github/pulls_test.go index f0a4854e090..a40291477ea 100644 --- a/github/pulls_test.go +++ b/github/pulls_test.go @@ -8,6 +8,7 @@ package github import ( "encoding/json" "fmt" + "io" "net/http" "reflect" "strings" @@ -232,31 +233,63 @@ func TestPullRequestsService_Create_invalidOwner(t *testing.T) { } func TestPullRequestsService_Edit(t *testing.T) { - setup() - defer teardown() + tests := []struct { + input *PullRequest + sendResponse string + + wantUpdate *pullRequestUpdate + wantResponse *PullRequest + }{ + { + input: &PullRequest{Title: String("t")}, + sendResponse: `{"number":1}`, + wantUpdate: &pullRequestUpdate{Title: String("t")}, + wantResponse: &PullRequest{Number: Int(1)}, + }, + { + // nil request + sendResponse: `{}`, + wantUpdate: &pullRequestUpdate{}, + wantResponse: &PullRequest{}, + }, + { + // base update + input: &PullRequest{Base: &PullRequestBranch{Ref: String("master")}}, + sendResponse: `{"number":1,"base":{"ref":"master"}}`, + wantUpdate: &pullRequestUpdate{Base: String("master")}, + wantResponse: &PullRequest{ + Number: Int(1), + Base: &PullRequestBranch{Ref: String("master")}, + }, + }, + } - input := &PullRequest{Title: String("t")} + for _, tt := range tests { + func() { + setup() + defer teardown() - mux.HandleFunc("/repos/o/r/pulls/1", func(w http.ResponseWriter, r *http.Request) { - v := new(PullRequest) - json.NewDecoder(r.Body).Decode(v) + mux.HandleFunc("/repos/o/r/pulls/1", func(w http.ResponseWriter, r *http.Request) { + v := new(pullRequestUpdate) + json.NewDecoder(r.Body).Decode(v) - testMethod(t, r, "PATCH") - if !reflect.DeepEqual(v, input) { - t.Errorf("Request body = %+v, want %+v", v, input) - } + testMethod(t, r, "PATCH") + if !reflect.DeepEqual(v, tt.wantUpdate) { + t.Errorf("Request body = %+v, want %+v", v, tt.wantUpdate) + } - fmt.Fprint(w, `{"number":1}`) - }) + io.WriteString(w, tt.sendResponse) + }) - pull, _, err := client.PullRequests.Edit("o", "r", 1, input) - if err != nil { - t.Errorf("PullRequests.Edit returned error: %v", err) - } + pull, _, err := client.PullRequests.Edit("o", "r", 1, tt.input) + if err != nil { + t.Errorf("PullRequests.Edit returned error: %v", err) + } - want := &PullRequest{Number: Int(1)} - if !reflect.DeepEqual(pull, want) { - t.Errorf("PullRequests.Edit returned %+v, want %+v", pull, want) + if !reflect.DeepEqual(pull, tt.wantResponse) { + t.Errorf("PullRequests.Edit returned %+v, want %+v", pull, tt.wantResponse) + } + }() } } From 8304959f4947f7c7716492e7a3c17dfaf200fa43 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Fri, 27 Jan 2017 22:12:50 -0800 Subject: [PATCH 2/5] pulls: Document Edit() better --- github/pulls.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/github/pulls.go b/github/pulls.go index c3dc2b73e89..9a161917dda 100644 --- a/github/pulls.go +++ b/github/pulls.go @@ -198,6 +198,9 @@ type pullRequestUpdate struct { // Edit a pull request. // +// The following fields are editable: Title, Body, State, and Base.Ref. +// Base.Ref updates the base branch of the pull request. +// // GitHub API docs: https://developer.github.com/v3/pulls/#update-a-pull-request func (s *PullRequestsService) Edit(owner string, repo string, number int, pull *PullRequest) (*PullRequest, *Response, error) { u := fmt.Sprintf("repos/%v/%v/pulls/%d", owner, repo, number) From 50056f339c6b34bcb1e56b1726aa1de05b6fbaa3 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Fri, 27 Jan 2017 22:25:01 -0800 Subject: [PATCH 3/5] pulls: Simplify Edit test --- github/pulls_test.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/github/pulls_test.go b/github/pulls_test.go index a40291477ea..54aa1be2ecc 100644 --- a/github/pulls_test.go +++ b/github/pulls_test.go @@ -9,6 +9,7 @@ import ( "encoding/json" "fmt" "io" + "io/ioutil" "net/http" "reflect" "strings" @@ -237,26 +238,26 @@ func TestPullRequestsService_Edit(t *testing.T) { input *PullRequest sendResponse string - wantUpdate *pullRequestUpdate + wantUpdate string wantResponse *PullRequest }{ { input: &PullRequest{Title: String("t")}, sendResponse: `{"number":1}`, - wantUpdate: &pullRequestUpdate{Title: String("t")}, + wantUpdate: `{"title":"t"}`, wantResponse: &PullRequest{Number: Int(1)}, }, { // nil request sendResponse: `{}`, - wantUpdate: &pullRequestUpdate{}, + wantUpdate: `{}`, wantResponse: &PullRequest{}, }, { // base update input: &PullRequest{Base: &PullRequestBranch{Ref: String("master")}}, sendResponse: `{"number":1,"base":{"ref":"master"}}`, - wantUpdate: &pullRequestUpdate{Base: String("master")}, + wantUpdate: `{"base":"master"}`, wantResponse: &PullRequest{ Number: Int(1), Base: &PullRequestBranch{Ref: String("master")}, @@ -270,12 +271,12 @@ func TestPullRequestsService_Edit(t *testing.T) { defer teardown() mux.HandleFunc("/repos/o/r/pulls/1", func(w http.ResponseWriter, r *http.Request) { - v := new(pullRequestUpdate) - json.NewDecoder(r.Body).Decode(v) - testMethod(t, r, "PATCH") - if !reflect.DeepEqual(v, tt.wantUpdate) { - t.Errorf("Request body = %+v, want %+v", v, tt.wantUpdate) + + gotUpdate, _ := ioutil.ReadAll(r.Body) + wantUpdate := tt.wantUpdate + "\n" // json encoder adds a newline + if string(gotUpdate) != wantUpdate { + t.Errorf("Request body = %q, want %q", gotUpdate, wantUpdate) } io.WriteString(w, tt.sendResponse) From 4651fa724483c28f36c0ad5a1c5600dfa62bf784 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 29 Jan 2017 11:40:09 -0800 Subject: [PATCH 4/5] pulls: Simplify test further Also test whether the request was acually made. --- github/pulls_test.go | 45 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/github/pulls_test.go b/github/pulls_test.go index 54aa1be2ecc..5058702d269 100644 --- a/github/pulls_test.go +++ b/github/pulls_test.go @@ -9,7 +9,6 @@ import ( "encoding/json" "fmt" "io" - "io/ioutil" "net/http" "reflect" "strings" @@ -234,6 +233,9 @@ func TestPullRequestsService_Create_invalidOwner(t *testing.T) { } func TestPullRequestsService_Edit(t *testing.T) { + setup() + defer teardown() + tests := []struct { input *PullRequest sendResponse string @@ -265,32 +267,27 @@ func TestPullRequestsService_Edit(t *testing.T) { }, } - for _, tt := range tests { - func() { - setup() - defer teardown() - - mux.HandleFunc("/repos/o/r/pulls/1", func(w http.ResponseWriter, r *http.Request) { - testMethod(t, r, "PATCH") - - gotUpdate, _ := ioutil.ReadAll(r.Body) - wantUpdate := tt.wantUpdate + "\n" // json encoder adds a newline - if string(gotUpdate) != wantUpdate { - t.Errorf("Request body = %q, want %q", gotUpdate, wantUpdate) - } + for i, tt := range tests { + madeRequest := false + mux.HandleFunc(fmt.Sprintf("/repos/o/r/pulls/%v", i), func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "PATCH") + testBody(t, r, tt.wantUpdate+"\n") + io.WriteString(w, tt.sendResponse) + madeRequest = true + }) - io.WriteString(w, tt.sendResponse) - }) + pull, _, err := client.PullRequests.Edit("o", "r", i, tt.input) + if err != nil { + t.Errorf("%d: PullRequests.Edit returned error: %v", i, err) + } - pull, _, err := client.PullRequests.Edit("o", "r", 1, tt.input) - if err != nil { - t.Errorf("PullRequests.Edit returned error: %v", err) - } + if !reflect.DeepEqual(pull, tt.wantResponse) { + t.Errorf("%d: PullRequests.Edit returned %+v, want %+v", i, pull, tt.wantResponse) + } - if !reflect.DeepEqual(pull, tt.wantResponse) { - t.Errorf("PullRequests.Edit returned %+v, want %+v", pull, tt.wantResponse) - } - }() + if !madeRequest { + t.Errorf("%d: PullRequest.Edit did not make the expected request", i) + } } } From 4ee5d1d561e4615a2a60074d43576151d9898d9e Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 29 Jan 2017 14:29:39 -0800 Subject: [PATCH 5/5] s/wantResponse/want/ --- github/pulls_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/github/pulls_test.go b/github/pulls_test.go index 5058702d269..f1d78bdc7ed 100644 --- a/github/pulls_test.go +++ b/github/pulls_test.go @@ -240,27 +240,27 @@ func TestPullRequestsService_Edit(t *testing.T) { input *PullRequest sendResponse string - wantUpdate string - wantResponse *PullRequest + wantUpdate string + want *PullRequest }{ { input: &PullRequest{Title: String("t")}, sendResponse: `{"number":1}`, wantUpdate: `{"title":"t"}`, - wantResponse: &PullRequest{Number: Int(1)}, + want: &PullRequest{Number: Int(1)}, }, { // nil request sendResponse: `{}`, wantUpdate: `{}`, - wantResponse: &PullRequest{}, + want: &PullRequest{}, }, { // base update input: &PullRequest{Base: &PullRequestBranch{Ref: String("master")}}, sendResponse: `{"number":1,"base":{"ref":"master"}}`, wantUpdate: `{"base":"master"}`, - wantResponse: &PullRequest{ + want: &PullRequest{ Number: Int(1), Base: &PullRequestBranch{Ref: String("master")}, }, @@ -281,8 +281,8 @@ func TestPullRequestsService_Edit(t *testing.T) { t.Errorf("%d: PullRequests.Edit returned error: %v", i, err) } - if !reflect.DeepEqual(pull, tt.wantResponse) { - t.Errorf("%d: PullRequests.Edit returned %+v, want %+v", i, pull, tt.wantResponse) + if !reflect.DeepEqual(pull, tt.want) { + t.Errorf("%d: PullRequests.Edit returned %+v, want %+v", i, pull, tt.want) } if !madeRequest {