Skip to content

Commit

Permalink
Add FilesOptions to ListFiles, ListFilesReviewed.
Browse files Browse the repository at this point in the history
Modify ListFilesReviewed to return a slice of strings, rather than slice
of FileInfos. This matches its actual behavior.

Modify both ListFiles and ListFilesReviewed to return map/slice directly,
rather than a pointer to one. There doesn't appear to be any value in
returning a pointer, it just makes the API harder to use. Slice/map are
already reference types.

Modify all endpoints to escape fileID parameter so it can be safely
placed inside a URL path segment.

Make note of Base parameter being undocumented for ListFiles endpoints.
However, it has been tested and it works (it's a very important parameter
to support).

Add tests for ListFiles, ListFilesReviewed.
  • Loading branch information
dmitshur authored and andygrunwald committed Apr 29, 2018
1 parent 5ff0cbc commit 2e8da2e
Show file tree
Hide file tree
Showing 2 changed files with 137 additions and 22 deletions.
78 changes: 56 additions & 22 deletions changes_revision.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package gerrit

import (
"fmt"
"net/url"
)

// DiffInfo entity contains information about the diff of a file in a revision.
Expand Down Expand Up @@ -64,8 +65,14 @@ type DiffOptions struct {
// If the intraline parameter is specified, intraline differences are included in the diff.
Intraline bool `url:"intraline,omitempty"`

//The base parameter can be specified to control the base patch set from which the diff should be generated.
Base bool `url:"base,omitempty"`
// The base parameter can be specified to control the base patch set from which the diff
// should be generated.
Base string `url:"base,omitempty"`

// The integer-valued request parameter parent can be specified to control the parent commit number
// against which the diff should be generated. This is useful for supporting review of merge commits.
// The value is the 1-based index of the parent’s position in the commit object.
Parent int `url:"parent,omitempty"`

// If the weblinks-only parameter is specified, only the diff web links are returned.
WeblinksOnly bool `url:"weblinks-only,omitempty"`
Expand Down Expand Up @@ -94,6 +101,28 @@ type MergableOptions struct {
OtherBranches bool `url:"other-branches,omitempty"`
}

// FilesOptions specifies the parameters for ListFiles and ListFilesReviewed calls.
//
// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-files
type FilesOptions struct {
// The request parameter q changes the response to return a list of all files (modified or unmodified)
// that contain that substring in the path name. This is useful to implement suggestion services
// finding a file by partial name.
Q string `url:"q,omitempty"`

// The base parameter can be specified to control the base patch set from which the list of files
// should be generated.
//
// Note: This option is undocumented.
Base string `url:"base,omitempty"`

// The integer-valued request parameter parent changes the response to return a list of the files
// which are different in this commit compared to the given parent commit. This is useful for
// supporting review of merge commits. The value is the 1-based index of the parent’s position
// in the commit object.
Parent int `url:"parent,omitempty"`
}

// PatchOptions specifies the parameters for GetPatch call.
//
// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-patch
Expand All @@ -114,7 +143,7 @@ type PatchOptions struct {
//
// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-diff
func (s *ChangesService) GetDiff(changeID, revisionID, fileID string, opt *DiffOptions) (*DiffInfo, *Response, error) {
u := fmt.Sprintf("changes/%s/revisions/%s/files/%s/diff", changeID, revisionID, fileID)
u := fmt.Sprintf("changes/%s/revisions/%s/files/%s/diff", changeID, revisionID, url.PathEscape(fileID))

u, err := addOptions(u, opt)
if err != nil {
Expand Down Expand Up @@ -289,18 +318,21 @@ func (s *ChangesService) ListRevisionComments(changeID, revisionID string) (*map
// The entries in the map are sorted by file path.
//
// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-files
func (s *ChangesService) ListFiles(changeID, revisionID string) (*map[string]FileInfo, *Response, error) {
// TODO: Missing q parameter
// The request parameter q changes the response to return a list of all files (modified or unmodified) that contain that substring in the path name. This is useful to implement suggestion services finding a file by partial name.
func (s *ChangesService) ListFiles(changeID, revisionID string, opt *FilesOptions) (map[string]FileInfo, *Response, error) {
u := fmt.Sprintf("changes/%s/revisions/%s/files/", changeID, revisionID)

u, err := addOptions(u, opt)
if err != nil {
return nil, nil, err
}

req, err := s.client.NewRequest("GET", u, nil)
if err != nil {
return nil, nil, err
}

v := new(map[string]FileInfo)
resp, err := s.client.Do(req, v)
var v map[string]FileInfo
resp, err := s.client.Do(req, &v)
if err != nil {
return nil, resp, err
}
Expand All @@ -309,23 +341,25 @@ func (s *ChangesService) ListFiles(changeID, revisionID string) (*map[string]Fil
}

// ListFilesReviewed lists the files that were modified, added or deleted in a revision.
// The difference between ListFiles and ListFilesReviewed is that the caller has marked these files as reviewed.
// Clients that also need the FileInfo should make two requests.
// Unlike ListFiles, the response of ListFilesReviewed is a list of the paths the caller
// has marked as reviewed. Clients that also need the FileInfo should make two requests.
//
// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-files
func (s *ChangesService) ListFilesReviewed(changeID, revisionID string) (*[]FileInfo, *Response, error) {
// TODO: Missing q parameter
// The request parameter q changes the response to return a list of all files (modified or unmodified) that contain that substring in the path name. This is useful to implement suggestion services finding a file by partial name.
func (s *ChangesService) ListFilesReviewed(changeID, revisionID string, opt *FilesOptions) ([]string, *Response, error) {
u := fmt.Sprintf("changes/%s/revisions/%s/files/", changeID, revisionID)

opt := struct {
o := struct {
// The request parameter reviewed changes the response to return a list of the paths the caller has marked as reviewed.
Reviewed bool `url:"reviewed,omitempty"`

FilesOptions
}{
Reviewed: true,
}

u, err := addOptions(u, opt)
if opt != nil {
o.FilesOptions = *opt
}
u, err := addOptions(u, o)
if err != nil {
return nil, nil, err
}
Expand All @@ -335,8 +369,8 @@ func (s *ChangesService) ListFilesReviewed(changeID, revisionID string) (*[]File
return nil, nil, err
}

v := new([]FileInfo)
resp, err := s.client.Do(req, v)
var v []string
resp, err := s.client.Do(req, &v)
if err != nil {
return nil, resp, err
}
Expand Down Expand Up @@ -525,7 +559,7 @@ func (s *ChangesService) DeleteDraft(changeID, revisionID, draftID string) (*Res
//
// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#delete-reviewed
func (s *ChangesService) DeleteReviewed(changeID, revisionID, fileID string) (*Response, error) {
u := fmt.Sprintf("changes/%s/revisions/%s/files/%s/reviewed", changeID, revisionID, fileID)
u := fmt.Sprintf("changes/%s/revisions/%s/files/%s/reviewed", changeID, revisionID, url.PathEscape(fileID))
return s.client.DeleteRequest(u, nil)
}

Expand All @@ -536,7 +570,7 @@ func (s *ChangesService) DeleteReviewed(changeID, revisionID, fileID string) (*R
//
// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-content
func (s *ChangesService) GetContent(changeID, revisionID, fileID string) (*string, *Response, error) {
u := fmt.Sprintf("changes/%s/revisions/%s/files/%s/content", changeID, revisionID, fileID)
u := fmt.Sprintf("changes/%s/revisions/%s/files/%s/content", changeID, revisionID, url.PathEscape(fileID))

req, err := s.client.NewRequest("GET", u, nil)
if err != nil {
Expand All @@ -560,7 +594,7 @@ func (s *ChangesService) GetContent(changeID, revisionID, fileID string) (*strin
//
// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-content
func (s *ChangesService) GetContentType(changeID, revisionID, fileID string) (*Response, error) {
u := fmt.Sprintf("changes/%s/revisions/%s/files/%s/content", changeID, revisionID, fileID)
u := fmt.Sprintf("changes/%s/revisions/%s/files/%s/content", changeID, revisionID, url.PathEscape(fileID))

req, err := s.client.NewRequest("HEAD", u, nil)
if err != nil {
Expand All @@ -576,7 +610,7 @@ func (s *ChangesService) GetContentType(changeID, revisionID, fileID string) (*R
//
// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#set-reviewed
func (s *ChangesService) SetReviewed(changeID, revisionID, fileID string) (*Response, error) {
u := fmt.Sprintf("changes/%s/revisions/%s/files/%s/reviewed", changeID, revisionID, fileID)
u := fmt.Sprintf("changes/%s/revisions/%s/files/%s/reviewed", changeID, revisionID, url.PathEscape(fileID))

req, err := s.client.NewRequest("PUT", u, nil)
if err != nil {
Expand Down
81 changes: 81 additions & 0 deletions changes_revision_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package gerrit_test

import (
"fmt"
"net/http"
"net/http/httptest"
"reflect"
"testing"

"github.com/andygrunwald/go-gerrit"
)

func TestChangesService_ListFiles(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if got, want := r.URL.String(), "/changes/123/revisions/456/files/?base=7"; got != want {
t.Errorf("request URL:\ngot: %q\nwant: %q", got, want)
}
fmt.Fprint(w, `{
"/COMMIT_MSG": {
"status": "A",
"lines_inserted": 7,
"size_delta": 551,
"size": 551
},
"gerrit-server/RefControl.java": {
"lines_inserted": 5,
"lines_deleted": 3,
"size_delta": 98,
"size": 23348
}
}`)
}))
defer ts.Close()

client := newClient(t, ts)
got, _, err := client.Changes.ListFiles("123", "456", &gerrit.FilesOptions{
Base: "7",
})
if err != nil {
t.Fatal(err)
}
want := map[string]gerrit.FileInfo{
"/COMMIT_MSG": {
Status: "A",
LinesInserted: 7,
SizeDelta: 551,
Size: 551,
},
"gerrit-server/RefControl.java": {
LinesInserted: 5,
LinesDeleted: 3,
SizeDelta: 98,
Size: 23348,
},
}
if !reflect.DeepEqual(got, want) {
t.Errorf("client.Changes.ListFiles:\ngot: %+v\nwant: %+v", got, want)
}
}

func TestChangesService_ListFilesReviewed(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if got, want := r.URL.String(), "/changes/123/revisions/456/files/?q=abc&reviewed=true"; got != want {
t.Errorf("request URL:\ngot: %q\nwant: %q", got, want)
}
fmt.Fprint(w, `["/COMMIT_MSG","gerrit-server/RefControl.java"]`)
}))
defer ts.Close()

client := newClient(t, ts)
got, _, err := client.Changes.ListFilesReviewed("123", "456", &gerrit.FilesOptions{
Q: "abc",
})
if err != nil {
t.Fatal(err)
}
want := []string{"/COMMIT_MSG", "gerrit-server/RefControl.java"}
if !reflect.DeepEqual(got, want) {
t.Errorf("client.Changes.ListFilesReviewed:\ngot: %q\nwant: %q", got, want)
}
}

0 comments on commit 2e8da2e

Please sign in to comment.