Merge pull request #38 from andygrunwald/add-linters

Implement Static Analysis and Fix Bugs

.gitignore

@@ -1 +1 @@
-/coverage.txt
+/coverage.txt

.travis.yml

@@ -3,18 +3,15 @@ language: go
 sudo: false
 
 go:
-    - 1.5
     - 1.6
     - 1.7
     - 1.8
 
 before_install:
-    - go get github.com/axw/gocov/gocov
-    - if ! go get code.google.com/p/go.tools/cmd/cover; then go get golang.org/x/tools/cmd/cover; fi
-    - go get -t -v ./...
+  - make deps
 
 script:
-  - go test -race -coverprofile=coverage.txt -covermode=atomic
+  - make
 
 after_success:
   - bash <(curl -s https://codecov.io/bash)

CHANGELOG.md

@@ -9,6 +9,24 @@ first. For more complete details see
 
 ### latest (not yet released)
 
+### 0.4.0
+
+**WARNING**: This release includes breaking changes.
+
+* [BREAKING CHANGE] - Added gometalinter to the build and fixed problems 
+  discovered by the linters.
+    * Comment and error string fixes.
+    * Numerous lint and styling fixes.
+    * Ensured error values are being properly checked where appropriate.
+    * Addition of missing documentation
+    * Removed filePath parameter from DeleteChangeEdit which was unused and 
+      unnecessary for the request.
+    * Fixed CherryPickRevision and IncludeGroups functions which didn't pass
+      along the provided input structs into the request.
+* Go 1.5 has been removed from testing on Travis. The linters introduced in 
+  0.4.0 do not support this version, Go 1.5 is lacking security updates and
+  most Linux distros have moved beyond Go 1.5 now.
+
 ### 0.3.0
 
 **WARNING**: This release includes breaking changes.

Makefile

@@ -0,0 +1,26 @@
+.PHONY: fmt vet check-vendor lint check clean test build
+PACKAGES = $(shell go list ./...)
+PACKAGE_DIRS = $(shell go list -f '{{ .Dir }}' ./...)
+
+check: test vet lint
+
+test:
+	go test -v -race -coverprofile=coverage.txt -covermode=atomic ./...
+
+vet:
+	go vet $(PACKAGES) || (go clean $(PACKAGES); go vet $(PACKAGES))
+
+lint:
+	gometalinter --config gometalinter.json ./...
+
+fmt:
+	go fmt $(PACKAGES)
+	goimports -w $(PACKAGE_DIRS)
+
+deps:
+	go get -t -v ./...
+	go get github.com/axw/gocov/gocov
+	go get golang.org/x/tools/cmd/cover
+	[ -f $(GOPATH)/bin/gometalinter ] || go get -u github.com/alecthomas/gometalinter
+	[ -f $(GOPATH)/bin/goimports ] || go get golang.org/x/tools/cmd/goimports
+	gometalinter --install

README.md

@@ -32,13 +32,22 @@ It is go gettable ...
 $ go get github.com/andygrunwald/go-gerrit
 ```
 
-... (optional) to run unit / example tests:
+... (optional) to run checks and tests:
+
+**Tests Only**
 
 ```sh
 $ cd $GOPATH/src/github.com/andygrunwald/go-gerrit
 $ go test -v
 ```
 
+**Checks, Tests, Linters, etc**
+
+```sh
+$ cd $GOPATH/src/github.com/andygrunwald/go-gerrit
+$ make 
+``` 
+
 ## API / Usage
 
 Please have a look at the [GoDoc documentation](https://godoc.org/github.com/andygrunwald/go-gerrit) for a detailed API description.

authentication.go

@@ -1,7 +1,7 @@
 package gerrit
 
 import (
-	"crypto/md5"
+	"crypto/md5" // nolint: gas
 	"crypto/rand"
 	"encoding/base64"
 	"errors"
@@ -12,13 +12,13 @@ import (
 )
 
 var (
-	// Returned by digestAuthHeader when the WWW-Authenticate header is missing
+	// ErrWWWAuthenticateHeaderMissing is returned by digestAuthHeader when the WWW-Authenticate header is missing
 	ErrWWWAuthenticateHeaderMissing = errors.New("WWW-Authenticate header is missing")
 
-	// Returned by digestAuthHeader when the WWW-Authenticate invalid
+	// ErrWWWAuthenticateHeaderInvalid is returned by digestAuthHeader when the WWW-Authenticate invalid
 	ErrWWWAuthenticateHeaderInvalid = errors.New("WWW-Authenticate header is invalid")
 
-	// Returned by digestAuthHeader when the WWW-Authenticate header is not 'Digest'
+	// ErrWWWAuthenticateHeaderNotDigest is returned by digestAuthHeader when the WWW-Authenticate header is not 'Digest'
 	ErrWWWAuthenticateHeaderNotDigest = errors.New("WWW-Authenticate header type is not Digest")
 )
 
@@ -117,15 +117,19 @@ func (s *AuthenticationService) digestAuthHeader(response *http.Response) (strin
 	uriHeader := authenticate["uri"]
 
 	// A1
-	h := md5.New()
+	h := md5.New() // nolint: gas
 	A1 := fmt.Sprintf("%s:%s:%s", s.name, realmHeader, s.secret)
-	io.WriteString(h, A1)
+	if _, err := io.WriteString(h, A1); err != nil {
+		return "", err
+	}
 	HA1 := fmt.Sprintf("%x", h.Sum(nil))
 
 	// A2
-	h = md5.New()
+	h = md5.New() // nolint: gas
 	A2 := fmt.Sprintf("%s:%s", response.Request.Method, uriHeader)
-	io.WriteString(h, A2)
+	if _, err := io.WriteString(h, A2); err != nil {
+		return "", err
+	}
 	HA2 := fmt.Sprintf("%x", h.Sum(nil))
 
 	k := make([]byte, 12)
@@ -137,8 +141,10 @@ func (s *AuthenticationService) digestAuthHeader(response *http.Response) (strin
 		bytes += n
 	}
 	cnonce := base64.StdEncoding.EncodeToString(k)
-	digest := md5.New()
-	digest.Write([]byte(strings.Join([]string{HA1, nonceHeader, "00000001", cnonce, qopHeader, HA2}, ":")))
+	digest := md5.New() // nolint: gas
+	if _, err := digest.Write([]byte(strings.Join([]string{HA1, nonceHeader, "00000001", cnonce, qopHeader, HA2}, ":"))); err != nil {
+		return "", err
+	}
 	responseField := fmt.Sprintf("%x", digest.Sum(nil))
 
 	return fmt.Sprintf(

changes.go

@@ -675,6 +675,11 @@ func (s *ChangesService) FixChange(changeID string, input *FixInput) (*ChangeInf
 	return v, resp, err
 }
 
+// SubmitChange submits a change.
+//
+// The request body only needs to include a SubmitInput entity if submitting on behalf of another user.
+//
+// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#submit-change
 func (s *ChangesService) SubmitChange(changeID string, input *SubmitInput) (*ChangeInfo, *Response, error) {
 	u := fmt.Sprintf("changes/%s/submit", changeID)
 

changes_edit.go

@@ -140,8 +140,8 @@ func (s *ChangesService) DeleteFileInChangeEdit(changeID, filePath string) (*Res
 //
 // As response “204 No Content” is returned.
 //
-// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#delete-edit-file
-func (s *ChangesService) DeleteChangeEdit(changeID, filePath string) (*Response, error) {
+// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#delete-edit
+func (s *ChangesService) DeleteChangeEdit(changeID string) (*Response, error) {
 	u := fmt.Sprintf("changes/%s/edit", changeID)
 	return s.client.DeleteRequest(u, nil)
 }
@@ -151,7 +151,7 @@ func (s *ChangesService) DeleteChangeEdit(changeID, filePath string) (*Response,
 // As response “204 No Content” is returned.
 //
 // Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#publish-edit
-func (s *ChangesService) PublishChangeEdit(changeID, notify string)  (*Response, error) {
+func (s *ChangesService) PublishChangeEdit(changeID, notify string) (*Response, error) {
 	u := fmt.Sprintf("changes/%s/edit:publish", changeID)
 
 	req, err := s.client.NewRequest("POST", u, map[string]string{
@@ -160,7 +160,6 @@ func (s *ChangesService) PublishChangeEdit(changeID, notify string)  (*Response,
 	if err != nil {
 		return nil, err
 	}
-
 	return s.client.Do(req, nil)
 }
 

changes_revision.go

@@ -590,7 +590,7 @@ func (s *ChangesService) SetReviewed(changeID, revisionID, fileID string) (*Resp
 func (s *ChangesService) CherryPickRevision(changeID, revisionID string, input *CherryPickInput) (*ChangeInfo, *Response, error) {
 	u := fmt.Sprintf("changes/%s/revisions/%s/cherrypick", changeID, revisionID)
 
-	req, err := s.client.NewRequest("POST", u, nil)
+	req, err := s.client.NewRequest("POST", u, input)
 	if err != nil {
 		return nil, nil, err
 	}

changes_test.go

@@ -35,7 +35,7 @@ func ExampleChangesService_QueryChanges() {
 }
 
 // Prior to fixing #18 this test would fail.
-func ExampleChangesService_QueryChangesWithSymbols() {
+func ExampleChangesService_QueryChangesWithSymbols() { // nolint: vet
 	instance := "https://android-review.googlesource.com/"
 	client, err := gerrit.NewClient(instance, nil)
 	if err != nil {

events.go

@@ -143,15 +143,15 @@ func (events *EventsLogService) GetEvents(options *EventsLogOptions) ([]EventInf
 	}
 
 	body, err := ioutil.ReadAll(response.Body)
-	defer response.Body.Close()
+	defer response.Body.Close() // nolint: errcheck
 	if err != nil {
 		return info, response, failures, err
 	}
 
 	for _, line := range bytes.Split(body, []byte("\n")) {
 		if len(line) > 0 {
 			event := EventInfo{}
-			if err := json.Unmarshal(line, &event); err != nil {
+			if err := json.Unmarshal(line, &event); err != nil { // nolint: vetshadow
 				failures = append(failures, line)
 
 				if !options.IgnoreUnmarshalErrors {

events_test.go

@@ -24,7 +24,9 @@ func TestEventsLogService_GetEvents_NoDateRange(t *testing.T) {
 	defer teardown()
 
 	testMux.HandleFunc("/plugins/events-log/events/", func(writer http.ResponseWriter, request *http.Request) {
-		writer.Write(fakeEvents)
+		if _, err := writer.Write(fakeEvents); err != nil {
+			t.Error(err)
+		}
 	})
 
 	options := &gerrit.EventsLogOptions{}
@@ -72,7 +74,9 @@ func TestEventsLogService_GetEvents_DateRangeFromAndTo(t *testing.T) {
 			t.Errorf("%s != %s", query.Get("t2"), toFormat)
 		}
 
-		writer.Write(fakeEvents)
+		if _, err := writer.Write(fakeEvents); err != nil {
+			t.Error(err)
+		}
 	})
 
 	options := &gerrit.EventsLogOptions{From: from, To: to}
@@ -101,7 +105,9 @@ func TestEventsLogService_GetEvents_DateRangeFromOnly(t *testing.T) {
 			t.Error("Did not expect t2 to be set")
 		}
 
-		writer.Write(fakeEvents)
+		if _, err := writer.Write(fakeEvents); err != nil {
+			t.Error(err)
+		}
 	})
 
 	options := &gerrit.EventsLogOptions{From: from}
@@ -129,7 +135,9 @@ func TestEventsLogService_GetEvents_DateRangeToOnly(t *testing.T) {
 			t.Error("Did not expect t1 to be set")
 		}
 
-		writer.Write(fakeEvents)
+		if _, err := writer.Write(fakeEvents); err != nil {
+			t.Error(err)
+		}
 	})
 
 	options := &gerrit.EventsLogOptions{To: to}
@@ -144,7 +152,9 @@ func TestEventsLogService_GetEvents_UnmarshalError(t *testing.T) {
 	defer teardown()
 
 	testMux.HandleFunc("/", func(writer http.ResponseWriter, request *http.Request) {
-		writer.Write(fakeEventsWithError)
+		if _, err := writer.Write(fakeEventsWithError); err != nil {
+			t.Error(err)
+		}
 	})
 
 	options := &gerrit.EventsLogOptions{IgnoreUnmarshalErrors: true}