enable more linters in CI; fix some issues caught by them (#23)

.travis.yml

@@ -9,5 +9,4 @@ matrix:
     - go: tip
 
 script:
-  # TODO: change to "make" when golint, errcheck, staticcheck pass
+  - make
-  - make deps checkgofmt vet unused test

Makefile

@@ -1,64 +1,65 @@
-SRCS := $(shell find . -name '*.go')
+# TODO: run golint and errcheck, but only to catch *new* violations and
-PKGS := $(shell go list ./...)
+# decide whether to change code or not (e.g. we need to be able to whitelist
-
+# violations already in the code). They can be useful to catch errors, but
-.PHONY: all
+# they are just too noisy to be a requirement for a CI -- we don't even *want*
-all: deps lint test
+# to fix some of the things they consider to be violations.
+.PHONY: ci
+ci: deps checkgofmt vet staticcheck unused ineffassign predeclared test
 
 .PHONY: deps
 deps:
-	go get -d -v -t $(PKGS)
+	go get -d -v -t ./...
 
 .PHONY: updatedeps
 updatedeps:
-	go get -d -v -t -u -f $(PKGS)
+	go get -d -v -t -u -f ./...
 
 .PHONY: install
 install:
-	go install $(PKGS)
+	go install ./...
-
-.PHONY: golint
-golint:
-	@go get github.com/golang/lint/golint
-	for file in $(SRCS); do \
-		golint $${file}; \
-		if [ -n "$$(golint $${file})" ]; then \
-			exit 1; \
-		fi; \
-	done
 
 .PHONY: checkgofmt
 checkgofmt:
-	gofmt -s -l $(SRCS)
+	gofmt -s -l .
-	if [ -n "$$(gofmt -s -l $(SRCS))" ]; then \
+	@if [ -n "$$(gofmt -s -l .)" ]; then \
 		exit 1; \
 	fi
 
 .PHONY: vet
 vet:
-	go vet $(PKGS)
+	go vet ./...
-
-.PHONY:
-errcheck:
-	@go get github.com/kisielk/errcheck
-	errcheck $(PKGS)
 
 .PHONY: staticcheck
 staticcheck:
 	@go get honnef.co/go/tools/cmd/staticcheck
-	staticcheck $(PKGS)
+	staticcheck ./...
 
 .PHONY: unused
 unused:
 	@go get honnef.co/go/tools/cmd/unused
-	unused $(PKGS)
+	unused ./...
 
-.PHONY: lint
+.PHONY: ineffassign
-lint: golint checkgofmt vet errcheck staticcheck unused
+ineffassign:
+	@go get github.com/gordonklaus/ineffassign
+	ineffassign .
+
+.PHONY: predeclared
+	@go get github.com/nishanths/predeclared
+	predeclared .
+
+# Intentionally omitted from CI, but target here for ad-hoc reports.
+.PHONY: golint
+golint:
+	@go get github.com/golang/lint/golint
+	golint -min_confidence 0.9 -set_exit_status ./...
+
+# Intentionally omitted from CI, but target here for ad-hoc reports.
+.PHONY:
+errcheck:
+	@go get github.com/kisielk/errcheck
+	errcheck ./...
 
 .PHONY: test
 test:
-	go test -race $(PKGS)
+	go test -race ./...
-
-.PHONY: clean
-clean:
-	go clean -i $(PKGS)

cmd/grpcurl/grpcurl.go

@@ -235,7 +235,9 @@ func main() {
 				fail(err, "Failed to configure transport credentials")
 			}
 			if *serverName != "" {
-				creds.OverrideServerName(*serverName)
+				if err := creds.OverrideServerName(*serverName); err != nil {
+					fail(err, "Failed to override server name as %q", *serverName)
+				}
 			}
 		}
 		cc, err := grpcurl.BlockingDial(ctx, target, creds, opts...)
@@ -360,7 +362,10 @@ func main() {
 				tmpl := makeTemplate(dynamic.NewMessage(dsc))
 				fmt.Println("\nMessage template:")
 				jsm := jsonpb.Marshaler{Indent: "  ", EmitDefaults: true}
-				jsm.Marshal(os.Stdout, tmpl)
+				err := jsm.Marshal(os.Stdout, tmpl)
+				if err != nil {
+					fail(err, "Failed to print template for message %s", s)
+				}
 				fmt.Println()
 			}
 		}
@@ -476,11 +481,10 @@ func (h *handler) getRequestData() ([]byte, error) {
 	var msg json.RawMessage
 	if err := h.dec.Decode(&msg); err != nil {
 		return nil, err
-	} else {
+	}
 	h.reqCount++
 	return msg, nil
 }
-}
 
 func (*handler) OnReceiveHeaders(md metadata.MD) {
 	if *verbose {

grpcurl.go

@@ -304,9 +304,8 @@ func InvokeRpc(ctx context.Context, source DescriptorSource, cc *grpc.ClientConn
 	if err != nil {
 		if isNotFoundError(err) {
 			return fmt.Errorf("target server does not expose service %q", svc)
-		} else {
-			return fmt.Errorf("failed to query for service descriptor %q: %v", svc, err)
 		}
+		return fmt.Errorf("failed to query for service descriptor %q: %v", svc, err)
 	}
 	sd, ok := dsc.(*desc.ServiceDescriptor)
 	if !ok {
@@ -714,8 +713,11 @@ func fetchAllExtensions(source DescriptorSource, ext *dynamic.ExtensionRegistry,
 	alreadyFetched[msgTypeName] = true
 	if len(md.GetExtensionRanges()) > 0 {
 		fds, err := source.AllExtensionsForType(msgTypeName)
+		if err != nil {
+			return fmt.Errorf("failed to query for extensions of type %s: %v", msgTypeName, err)
+		}
 		for _, fd := range fds {
-			if err = ext.AddExtension(fd); err != nil {
+			if err := ext.AddExtension(fd); err != nil {
 				return fmt.Errorf("could not register extension %s of type %s: %v", fd.GetFullyQualifiedName(), msgTypeName, err)
 			}
 		}
@@ -765,9 +767,8 @@ func fullyConvertToDynamic(msgFact *dynamic.MessageFactory, msg proto.Message) (
 					newVal, err := fullyConvertToDynamic(msgFact, v.(proto.Message))
 					if err != nil {
 						return nil, err
-					} else {
-						dm.PutMapField(fd, k, newVal)
 					}
+					dm.PutMapField(fd, k, newVal)
 				}
 			}
 		} else if fd.IsRepeated() {
@@ -777,9 +778,8 @@ func fullyConvertToDynamic(msgFact *dynamic.MessageFactory, msg proto.Message) (
 					newVal, err := fullyConvertToDynamic(msgFact, e.(proto.Message))
 					if err != nil {
 						return nil, err
-					} else {
-						dm.SetRepeatedField(fd, i, newVal)
 					}
+					dm.SetRepeatedField(fd, i, newVal)
 				}
 			}
 		} else {
@@ -788,9 +788,8 @@ func fullyConvertToDynamic(msgFact *dynamic.MessageFactory, msg proto.Message) (
 				newVal, err := fullyConvertToDynamic(msgFact, v.(proto.Message))
 				if err != nil {
 					return nil, err
-				} else {
-					dm.SetField(fd, newVal)
 				}
+				dm.SetField(fd, newVal)
 			}
 		}
 	}
@@ -936,9 +935,8 @@ func BlockingDial(ctx context.Context, address string, creds credentials.Transpo
 	case res := <-result:
 		if conn, ok := res.(*grpc.ClientConn); ok {
 			return conn, nil
-		} else {
-			return nil, res.(error)
 		}
+		return nil, res.(error)
 	case <-ctx.Done():
 		return nil, ctx.Err()
 	}

testing/test_server.go

@@ -15,9 +15,10 @@ import (
 	"github.com/fullstorydev/grpcurl"
 )
 
+// TestServer implements the TestService interface defined in example.proto.
 type TestServer struct{}
 
-// One empty request followed by one empty response.
+// EmptyCall accepts one empty request and issues one empty response.
 func (TestServer) EmptyCall(ctx context.Context, req *grpc_testing.Empty) (*grpc_testing.Empty, error) {
 	headers, trailers, failEarly, failLate := processMetadata(ctx)
 	grpc.SetHeader(ctx, headers)
@@ -32,8 +33,8 @@ func (TestServer) EmptyCall(ctx context.Context, req *grpc_testing.Empty) (*grpc
 	return req, nil
 }
 
-// One request followed by one response.
+// UnaryCall accepts one request and issues one response. The response includes
-// The server returns the client payload as-is.
+// the client's payload as-is.
 func (TestServer) UnaryCall(ctx context.Context, req *grpc_testing.SimpleRequest) (*grpc_testing.SimpleResponse, error) {
 	headers, trailers, failEarly, failLate := processMetadata(ctx)
 	grpc.SetHeader(ctx, headers)
@@ -50,8 +51,9 @@ func (TestServer) UnaryCall(ctx context.Context, req *grpc_testing.SimpleRequest
 	}, nil
 }
 
-// One request followed by a sequence of responses (streamed download).
+// StreamingOutputCall accepts one request and issues a sequence of responses
-// The server returns the payload with client desired type and sizes.
+// (streamed download). The server returns the payload with client desired type
+// and sizes as specified in the request's ResponseParameters.
 func (TestServer) StreamingOutputCall(req *grpc_testing.StreamingOutputCallRequest, str grpc_testing.TestService_StreamingOutputCallServer) error {
 	headers, trailers, failEarly, failLate := processMetadata(str.Context())
 	str.SetHeader(headers)
@@ -87,8 +89,9 @@ func (TestServer) StreamingOutputCall(req *grpc_testing.StreamingOutputCallReque
 	return nil
 }
 
-// A sequence of requests followed by one response (streamed upload).
+// StreamingInputCall accepts a sequence of requests and issues one response
-// The server returns the aggregated size of client payload as the result.
+// (streamed upload). The server returns the aggregated size of client payloads
+// as the result.
 func (TestServer) StreamingInputCall(str grpc_testing.TestService_StreamingInputCallServer) error {
 	headers, trailers, failEarly, failLate := processMetadata(str.Context())
 	str.SetHeader(headers)
@@ -121,9 +124,9 @@ func (TestServer) StreamingInputCall(str grpc_testing.TestService_StreamingInput
 	return nil
 }
 
-// A sequence of requests with each request served by the server immediately.
+// FullDuplexCall accepts a sequence of requests with each request served by the
-// As one request could lead to multiple responses, this interface
+// server immediately. As one request could lead to multiple responses, this
-// demonstrates the idea of full duplexing.
+// interface demonstrates the idea of full duplexing.
 func (TestServer) FullDuplexCall(str grpc_testing.TestService_FullDuplexCallServer) error {
 	headers, trailers, failEarly, failLate := processMetadata(str.Context())
 	str.SetHeader(headers)
@@ -163,10 +166,10 @@ func (TestServer) FullDuplexCall(str grpc_testing.TestService_FullDuplexCallServ
 	return nil
 }
 
-// A sequence of requests followed by a sequence of responses.
+// HalfDuplexCall accepts a sequence of requests and issues a sequence of
-// The server buffers all the client requests and then serves them in order. A
+// responses. The server buffers all the client requests and then serves them
-// stream of responses are returned to the client when the server starts with
+// in order. A stream of responses is returned to the client once the client
-// first request.
+// half-closes the stream.
 func (TestServer) HalfDuplexCall(str grpc_testing.TestService_HalfDuplexCallServer) error {
 	headers, trailers, failEarly, failLate := processMetadata(str.Context())
 	str.SetHeader(headers)
@@ -204,9 +207,25 @@ func (TestServer) HalfDuplexCall(str grpc_testing.TestService_HalfDuplexCallServ
 }
 
 const (
+	// MetadataReplyHeaders is a request header that contains values that will
+	// be echoed back to the client as response headers. The format of the value
+	// is "key: val". To have the server reply with more than one response
+	// header, supply multiple values in request metadata.
 	MetadataReplyHeaders = "reply-with-headers"
+	// MetadataReplyTrailers is a request header that contains values that will
+	// be echoed back to the client as response trailers. Its format its the
+	// same as MetadataReplyHeaders.
 	MetadataReplyTrailers = "reply-with-trailers"
+	// MetadataFailEarly is a request header that, if present and not zero,
+	// indicates that the RPC should fail immediately with that code.
 	MetadataFailEarly = "fail-early"
+	// MetadataFailLate is a request header that, if present and not zero,
+	// indicates that the RPC should fail at the end with that code. This is
+	// different from MetadataFailEarly only for streaming calls. An early
+	// failure means the call to fail before any request stream is read or any
+	// response stream is generated. A late failure means the entire request and
+	// response streams will be consumed/processed and only then will the error
+	// code be sent.
 	MetadataFailLate = "fail-late"
 )