Switched to revive, goimports, re-formatted everything (#1233)

* Switched to revive, goimports, re-formatted everything

Works pretty well!
Using latest tools/go.mod versions (except thrift), and using the server's revive.toml for starting out.

Revive is a bit noisy due to some `Id` -> `ID` recommendations, but we probably should actually do that for v2.

Makefile

@@ -142,8 +142,11 @@ $(BIN)/thriftrw: internal/tools/go.mod
 $(BIN)/thriftrw-plugin-yarpc: internal/tools/go.mod
 	$(call go_build_tool,go.uber.org/yarpc/encoding/thrift/thriftrw-plugin-yarpc)
 
-$(BIN)/golint: internal/tools/go.mod
-	$(call go_build_tool,golang.org/x/lint/golint)
+$(BIN)/goimports: internal/tools/go.mod
+	$(call go_build_tool,golang.org/x/tools/cmd/goimports)
+
+$(BIN)/revive: internal/tools/go.mod
+	$(call go_build_tool,github.com/mgechev/revive)
 
 $(BIN)/staticcheck: internal/tools/go.mod
 	$(call go_build_tool,honnef.co/go/tools/cmd/staticcheck)
@@ -217,27 +220,11 @@ $(THRIFT_GEN): $(THRIFT_FILES) $(BIN)/thriftrw $(BIN)/thriftrw-plugin-yarpc
 # other intermediates
 # ====================================
 
-# golint fails to report many lint failures if it is only given a single file
-# to work on at a time, and it can't handle multiple packages at once, *and*
-# we can't exclude files from its checks, so for best results we need to give
-# it a whitelist of every file in every package that we want linted, per package.
-#
-# so lint + this golint func works like:
-# - iterate over all lintable dirs (outputs "./folder/")
-# - find .go files in a dir (via wildcard, so not recursively)
-# - filter to only files in LINT_SRC
-# - if it's not empty, run golint against the list
-define lint_if_present
-test -n "$1" && $(BIN)/golint -set_exit_status $1
-endef
-
-# TODO: replace this with revive, like the server.
-# keep in sync with `lint`
-$(BUILD)/lint: $(BIN)/golint $(ALL_SRC)
-	$Q $(foreach pkg, \
-		$(sort $(dir $(LINT_SRC))), \
-		$(call lint_if_present,$(filter $(wildcard $(pkg)*.go),$(LINT_SRC))) || ERR=1; \
-	) test -z "$$ERR"; touch $@; exit $$ERR
+# note that LINT_SRC is fairly fake as a prerequisite.
+# it's a coarse "you probably don't need to re-lint" filter, nothing more.
+$(BUILD)/lint: $(LINT_SRC) $(BIN)/revive | $(BUILD)
+	$Q $(BIN)/revive -config revive.toml -exclude './vendor/...' -exclude './.gen/...' -formatter stylish ./...
+	$Q touch $@
 
 # fmt and copyright are mutually cyclic with their inputs, so if a copyright header is modified:
 # - copyright -> makes changes
@@ -253,11 +240,10 @@ $(BUILD)/lint: $(BIN)/golint $(ALL_SRC)
 MAYBE_TOUCH_COPYRIGHT=
 
 # TODO: switch to goimports, so we can pin the version
-$(BUILD)/fmt: $(ALL_SRC)
-	$Q echo "gofmt..."
+$(BUILD)/fmt: $(ALL_SRC) $(BIN)/goimports
+	$Q echo "goimports..."
 	$Q # use FRESH_ALL_SRC so it won't miss any generated files produced earlier
-	$Q gofmt -w $(ALL_SRC)
-	$Q # ideally, mimic server: $(BIN)/goimports -local "go.uber.org/cadence" -w $(FRESH_ALL_SRC)
+	$Q $(BIN)/goimports -local "go.uber.org/cadence" -w $(FRESH_ALL_SRC)
 	$Q touch $@
 	$Q $(MAYBE_TOUCH_COPYRIGHT)
 
@@ -274,22 +260,25 @@ $(BUILD)/copyright: $(ALL_SRC) $(BIN)/copyright
 # this way the effort is shared with future `make` runs.
 # ====================================
 
+# "re-make" a target by deleting and re-building book-keeping target(s).
+# the + is necessary for parallelism flags to be propagated
+define remake
+$Q rm -f $(addprefix $(BUILD)/,$(1))
+$Q +$(MAKE) --no-print-directory $(addprefix $(BUILD)/,$(1))
+endef
+
 .PHONY: build
 build: $(BUILD)/dummy ## ensure all packages build
 
 .PHONY: lint
-# useful to actually re-run to get output again, as the intermediate will not be run unless necessary.
-# dummy is used only because it occurs before $(BUILD)/lint, fmt would likely be sufficient too.
-# keep in sync with `$(BUILD)/lint`
-lint: $(BUILD)/dummy $(BIN)/golint ## (re)run golint
-	$Q $(foreach pkg, \
-		$(sort $(dir $(LINT_SRC))), \
-		$(call lint_if_present,$(filter $(wildcard $(pkg)*.go),$(LINT_SRC))) || ERR=1; \
-	) test -z "$$ERR"; touch $(BUILD)/lint; exit $$ERR
+# useful to actually re-run to get output again.
+# reuse the intermediates for simplicity and consistency.
+lint: ## (re)run the linter
+	$(call remake,lint)
 
 .PHONY: fmt
-# intentionally not re-making, gofmt is slow and it's clear when it's unnecessary
-fmt: $(BUILD)/fmt ## run gofmt
+# intentionally not re-making, it's clear when it's unnecessary
+fmt: $(BUILD)/fmt ## run goimports
 
 .PHONY: copyright
 # not identical to the intermediate target, but does provide the same codegen (or more).

activity/activity.go

@@ -24,8 +24,9 @@ import (
 	"context"
 
 	"github.com/uber-go/tally"
-	"go.uber.org/cadence/internal"
 	"go.uber.org/zap"
+
+	"go.uber.org/cadence/internal"
 )
 
 type (

compatibility/thrift2proto.go

@@ -21,9 +21,10 @@
 package compatibility
 
 import (
-	apiv1 "github.com/uber/cadence-idl/go/proto/api/v1"
 	"go.uber.org/cadence/.gen/go/cadence/workflowserviceclient"
 	internal "go.uber.org/cadence/internal/compatibility"
+
+	apiv1 "github.com/uber/cadence-idl/go/proto/api/v1"
 )
 
 // NewThrift2ProtoAdapter creates an adapter for mapping calls from Thrift to Protobuf types.

evictiontest/workflow_cache_eviction_test.go

@@ -37,14 +37,15 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/suite"
 	"go.uber.org/atomic"
+	"go.uber.org/yarpc"
+	"go.uber.org/zap/zaptest"
+	"golang.org/x/net/context"
+
 	"go.uber.org/cadence/.gen/go/cadence/workflowservicetest"
 	m "go.uber.org/cadence/.gen/go/shared"
 	"go.uber.org/cadence/internal"
 	"go.uber.org/cadence/internal/common"
 	"go.uber.org/cadence/worker"
-	"go.uber.org/yarpc"
-	"go.uber.org/zap/zaptest"
-	"golang.org/x/net/context"
 )
 
 // copied from internal/test_helpers_test.go

internal/activity.go

@@ -28,9 +28,10 @@ import (
 
 	"github.com/opentracing/opentracing-go"
 	"github.com/uber-go/tally"
-	"go.uber.org/cadence/.gen/go/shared"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
+
+	"go.uber.org/cadence/.gen/go/shared"
 )
 
 type (

internal/activity_test.go

@@ -27,10 +27,11 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
+	"go.uber.org/yarpc"
+
 	"go.uber.org/cadence/.gen/go/cadence/workflowservicetest"
 	"go.uber.org/cadence/.gen/go/shared"
 	"go.uber.org/cadence/internal/common"
-	"go.uber.org/yarpc"
 )
 
 type activityTestSuite struct {

internal/client.go

@@ -28,11 +28,12 @@ import (
 
 	"github.com/opentracing/opentracing-go"
 	"github.com/uber-go/tally"
+	"go.uber.org/zap"
+
 	"go.uber.org/cadence/.gen/go/cadence/workflowserviceclient"
 	s "go.uber.org/cadence/.gen/go/shared"
 	"go.uber.org/cadence/internal/common/auth"
 	"go.uber.org/cadence/internal/common/metrics"
-	"go.uber.org/zap"
 )
 
 const (

internal/common/auth/service_wrapper.go

@@ -23,9 +23,10 @@ package auth
 import (
 	"context"
 
+	"go.uber.org/yarpc"
+
 	"go.uber.org/cadence/.gen/go/cadence/workflowserviceclient"
 	"go.uber.org/cadence/.gen/go/shared"
-	"go.uber.org/yarpc"
 )
 
 const (

internal/common/auth/service_wrapper_test.go

@@ -22,15 +22,15 @@ package auth
 
 import (
 	"fmt"
-	"github.com/uber/tchannel-go/thrift"
-	"go.uber.org/cadence/.gen/go/shared"
 	"testing"
 	"time"
 
 	"github.com/golang/mock/gomock"
-	"go.uber.org/cadence/.gen/go/cadence/workflowservicetest"
-
 	"github.com/stretchr/testify/suite"
+	"github.com/uber/tchannel-go/thrift"
+
+	"go.uber.org/cadence/.gen/go/cadence/workflowservicetest"
+	"go.uber.org/cadence/.gen/go/shared"
 )
 
 type (

internal/common/backoff/retry_test.go

@@ -27,6 +27,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+
 	"go.uber.org/cadence/.gen/go/shared"
 )
 

internal/common/metrics/service_wrapper.go

@@ -26,9 +26,10 @@ import (
 	"time"
 
 	"github.com/uber-go/tally"
+	"go.uber.org/yarpc"
+
 	"go.uber.org/cadence/.gen/go/cadence/workflowserviceclient"
 	"go.uber.org/cadence/.gen/go/shared"
-	"go.uber.org/yarpc"
 )
 
 type (

internal/common/metrics/service_wrapper_test.go

@@ -32,10 +32,11 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/uber-go/tally"
 	"github.com/uber/tchannel-go/thrift"
+	"go.uber.org/yarpc"
+
 	"go.uber.org/cadence/.gen/go/cadence/workflowserviceclient"
 	"go.uber.org/cadence/.gen/go/cadence/workflowservicetest"
 	s "go.uber.org/cadence/.gen/go/shared"
-	"go.uber.org/yarpc"
 )
 
 var (

internal/common/serializer/history_serializer.go

@@ -25,9 +25,11 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"go.uber.org/cadence/.gen/go/shared"
+
 	"go.uber.org/thriftrw/protocol"
 	"go.uber.org/thriftrw/wire"
+
+	"go.uber.org/cadence/.gen/go/shared"
 )
 
 type (