Skip to content

Add function for PullRequestService to download raw content of pull r… #481

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ const (
mediaTypeV3 = "application/vnd.github.v3+json"
defaultMediaType = "application/octet-stream"
mediaTypeV3SHA = "application/vnd.github.v3.sha"
mediaTypeV3Diff = "application/vnd.github.v3.diff"
mediaTypeV3Patch = "application/vnd.github.v3.patch"
mediaTypeOrgPermissionRepo = "application/vnd.github.v3.repository+json"

// Media Type values to access preview APIs
Expand Down Expand Up @@ -154,6 +156,20 @@ type UploadOptions struct {
Name string `url:"name,omitempty"`
}

// RawType represents type of raw format of a request instead of JSON.
type RawType uint8

const (
Diff RawType = 1 + iota
Patch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes 2 golint issues:

github.go:163:2: exported const Diff should have comment (or a comment on this block) or be unexported
github.go:164:2: exported const Patch should have comment (or a comment on this block) or be unexported

)

// RawOptions specifies parameters when user wants to get raw format of
// a response instead of JSON.
type RawOptions struct {
Type RawType
}

// addOptions adds the parameters in opt as URL query parameters to s. opt
// must be a struct whose fields may contain "url" tags.
func addOptions(s string, opt interface{}) (string, error) {
Expand Down
27 changes: 27 additions & 0 deletions github/pulls.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package github

import (
"bytes"
"fmt"
"time"
)
Expand Down Expand Up @@ -134,6 +135,32 @@ func (s *PullRequestsService) Get(owner string, repo string, number int) (*PullR
return pull, resp, err
}

// GetRaw gets raw (diff or patch) format of a pull request.
func (s *PullRequestsService) GetRaw(owner string, repo string, number int, opt RawOptions) (string, *Response, error) {
u := fmt.Sprintf("repos/%v/%v/pulls/%d", owner, repo, number)
req, err := s.client.NewRequest("GET", u, nil)
if err != nil {
return "", nil, err
}

switch opt.Type {
case Diff:
req.Header.Set("Accept", mediaTypeV3Diff)
case Patch:
req.Header.Set("Accept", mediaTypeV3Patch)
default:
return "", nil, fmt.Errorf("unsupported raw type %d", opt.Type)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a switch here, it would look nicer.

Copy link
Member

@dmitshur dmitshur Dec 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here.

  1. A switch statement would be a good fit here.
  2. How about handling when opt.Type value is not any of the supported types? I think returning an error would be most appropriate. Currently, it would just perform undefined actions.


ret := new(bytes.Buffer)
resp, err := s.client.Do(req, ret)
if err != nil {
return "", resp, err
}

return ret.String(), resp, err
}

// NewPullRequest represents a new pull request to be created.
type NewPullRequest struct {
Title *string `json:"title,omitempty"`
Expand Down
55 changes: 55 additions & 0 deletions github/pulls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"fmt"
"net/http"
"reflect"
"strings"
"testing"
)

Expand Down Expand Up @@ -67,6 +68,60 @@ func TestPullRequestsService_Get(t *testing.T) {
}
}

func TestPullRequestService_GetRawPatch(t *testing.T) {
setup()
defer teardown()
const rawStr = "@@patch content"

mux.HandleFunc("/repos/o/r/pulls/1", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
testHeader(t, r, "Accept", mediaTypeV3Patch)
fmt.Fprint(w, rawStr)
})

ret, _, err := client.PullRequests.GetRaw("o", "r", 1, RawOptions{Patch})
Copy link
Member

@dmitshur dmitshur Dec 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client code will not be able to write github.RawOptions{github.Patch}, because that'd cause a vet issue of composite literal using unkeyed fields.

They'd have to write github.RawOptions{Type: github.Patch}.

On the one hand, keeping the RawOptions struct means we can add additional options in the future, but on the other hand, it's more verbose than just passing a RawType parameter without a struct.

Just making an observation here. Not asking to change anything.

if err != nil {
t.Errorf("PullRequests.GetRaw return error: %v", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

}

if ret != rawStr {
t.Errorf("PullRequests.GetRaw returned %s want %s", ret, rawStr)
}
}

func TestPullRequestService_GetRawDiff(t *testing.T) {
setup()
defer teardown()
const rawStr = "@@diff content"

mux.HandleFunc("/repos/o/r/pulls/1", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
testHeader(t, r, "Accept", mediaTypeV3Diff)
fmt.Fprint(w, rawStr)
})

ret, _, err := client.PullRequests.GetRaw("o", "r", 1, RawOptions{Diff})
if err != nil {
t.Errorf("PullRequests.GetRaw return error: %v", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too, this should be t.Fatalf.

}

if ret != rawStr {
t.Errorf("PullRequests.GetRaw returned %s want %s", ret, rawStr)
}
}

func TestPullRequestService_GetRawInvalid(t *testing.T) {
setup()
defer teardown()
_, _, err := client.PullRequests.GetRaw("o", "r", 1, RawOptions{100})
if err == nil {
t.Error("PullRequests.GetRaw should return error")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If err == nil, it doesn't make sense to continue to the next check that looks at err.Error().

So, change t.Error to t.Fatal here.

}
if !strings.Contains(err.Error(), "unsupported raw type") {
t.Error("PullRequests.GetRaw should return unsupported raw type error")
}
}

func TestPullRequestsService_Get_headAndBase(t *testing.T) {
setup()
defer teardown()
Expand Down