Skip to content
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

gometalinter -> golangci-lint migration #3933

Merged
merged 15 commits into from
Mar 19, 2019
Merged
Show file tree
Hide file tree
Changes from 7 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
8 changes: 1 addition & 7 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,17 +79,11 @@ jobs:
at: /tmp/workspace
- checkout
- *dependencies
- run:
name: Get metalinter
command: |
export PATH="$GOBIN:$PATH"
make devtools-clean
make devtools
- run:
name: Lint source
command: |
export PATH="$GOBIN:$PATH"
make test_lint
make lint

integration_tests:
<<: *linux_defaults
Expand Down
17 changes: 17 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
linters:
disable-all: true
enable:
- errcheck
- golint
- ineffassign
- unconvert
- misspell
linters-settings:
gocyclo:
min-complexity: 11
errcheck:
ignore: fmt:.*,io/ioutil:^Read.*,github.com/spf13/cobra:MarkFlagRequired,github.com/spf13/viper:BindPFlag
golint:
min-confidence: 1.1
run:
tests: false
1 change: 1 addition & 0 deletions .pending/improvements/sdk/Fixed-various-linter
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed various linters warnings in the context of the gometalinter -> golangci-lint migration #3896.
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ only pull requests targeted directly against master.
### Development Procedure:
- the latest state of development is on `develop`
- `develop` must never fail `make test` or `make test_cli`
- `develop` should not fail `make test_lint`
- `develop` should not fail `make lint`
- no --force onto `develop` (except when reverting a broken commit, which should seldom happen)
- create a development branch either on github.com/cosmos/cosmos-sdk, or your fork (using `git remote add origin`)
- before submitting a pull request, begin `git rebase` on top of `develop`
Expand Down
52 changes: 12 additions & 40 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@ VERSION := $(shell echo $(shell git describe --tags) | sed 's/^v//')
COMMIT := $(shell git log -1 --format='%H')
CAT := $(if $(filter $(OS),Windows_NT),type,cat)
LEDGER_ENABLED ?= true
GOTOOLS = \
github.com/alecthomas/gometalinter \
github.com/rakyll/statik
GOBIN ?= $(GOPATH)/bin
GOSUM := $(shell which gosum)

Expand Down Expand Up @@ -62,15 +59,15 @@ ldflags := $(strip $(ldflags))

BUILD_FLAGS := -tags "$(build_tags)" -ldflags '$(ldflags)'

all: devtools install test_lint test
all: tools install lint test

# The below include contains the tools target.
include scripts/Makefile

########################################
### CI

ci: devtools install test_cover test_lint test
ci: tools install test_cover lint test

########################################
### Build/Install
Expand Down Expand Up @@ -108,37 +105,12 @@ dist:
########################################
### Tools & dependencies

check_tools:
@# https://stackoverflow.com/a/25668869
@echo "Found tools: $(foreach tool,$(notdir $(GOTOOLS)),\
$(if $(shell which $(tool)),$(tool),$(error "No $(tool) in PATH")))"

update_tools:
@echo "--> Updating tools to correct version"
$(MAKE) --always-make tools

update_dev_tools:
@echo "--> Downloading linters (this may take awhile)"
$(GOPATH)/src/github.com/alecthomas/gometalinter/scripts/install.sh -b $(GOBIN)
go get -u github.com/tendermint/lint/golint

devtools: devtools-stamp
devtools-stamp: tools
@echo "--> Downloading linters (this may take awhile)"
$(GOPATH)/src/github.com/alecthomas/gometalinter/scripts/install.sh -b $(GOBIN)
go get github.com/tendermint/lint/golint
go install -mod=readonly ./cmd/sdkch
touch $@

devtools-clean: tools-clean
rm -f devtools-stamp

go-mod-cache: go.sum
@echo "--> Download go modules to local cache"
@go mod download

go.sum: tools go.mod
@echo "--> Generating vendor directory via go mod vendor"
@echo "--> Ensure dependencies have not been modified"
@go mod verify

draw_deps: tools
Expand All @@ -147,7 +119,7 @@ draw_deps: tools
@goviz -i github.com/cosmos/cosmos-sdk/cmd/gaia/cmd/gaiad -d 2 | dot -Tpng -o dependency-graph.png

clean:
rm -f devtools-stamp snapcraft-local.yaml
rm -f snapcraft-local.yaml

distclean: clean
rm -rf vendor/
Expand Down Expand Up @@ -227,13 +199,13 @@ test_sim_gaia_profile:
test_cover:
@export VERSION=$(VERSION); bash -x tests/test_cover.sh

test_lint:
gometalinter --config=tools/gometalinter.json ./...
!(gometalinter --exclude /usr/lib/go/src/ --exclude client/lcd/statik/statik.go --exclude 'vendor/*' --disable-all --enable='errcheck' --vendor ./... | grep -v "client/")
lint: tools
alessio marked this conversation as resolved.
Show resolved Hide resolved
golangci-lint run
go vet -composites=false -tests=false ./...
find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" | xargs gofmt -d -s
go mod verify

format:
format: tools
find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" -not -path "./client/lcd/statik/statik.go" | xargs gofmt -w -s
find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" -not -path "./client/lcd/statik/statik.go" | xargs misspell -w
find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" -not -path "./client/lcd/statik/statik.go" | xargs goimports -w -local github.com/cosmos/cosmos-sdk
Expand Down Expand Up @@ -292,10 +264,10 @@ snapcraft-local.yaml: snapcraft-local.yaml.in
# unless there is a reason not to.
# https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html
.PHONY: build install install_debug dist clean distclean \
check_tools check_dev_tools get_vendor_deps draw_deps test test_cli test_unit \
test_cover test_lint benchmark devdoc_init devdoc devdoc_save devdoc_update \
draw_deps test test_cli test_unit \
alessio marked this conversation as resolved.
Show resolved Hide resolved
test_cover lint benchmark devdoc_init devdoc devdoc_save devdoc_update \
build-linux build-docker-gaiadnode localnet-start localnet-stop \
format check-ledger test_sim_gaia_nondeterminism test_sim_modules test_sim_gaia_fast \
test_sim_gaia_custom_genesis_fast test_sim_gaia_custom_genesis_multi_seed \
test_sim_gaia_multi_seed test_sim_gaia_import_export update_tools update_dev_tools \
devtools-clean go-mod-cache
test_sim_gaia_multi_seed test_sim_gaia_import_export \
go-mod-cache
24 changes: 24 additions & 0 deletions client/keys/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,17 @@ type AddNewKey struct {
Index int `json:"index,string,omitempty"`
}

// NewAddNewKey constructs a new AddNewKey request structure.
func NewAddNewKey(name, password, mnemonic string, account, index int) AddNewKey {
alessio marked this conversation as resolved.
Show resolved Hide resolved
return AddNewKey{
Name: name,
Password: password,
Mnemonic: mnemonic,
Account: account,
Index: index,
}
}

// RecoverKeyBody recovers a key
type RecoverKey struct {
Password string `json:"password"`
Expand All @@ -19,13 +30,26 @@ type RecoverKey struct {
Index int `json:"index,string,omitempty"`
}

// NewRecoverKey constructs a new RecoverKey request structure.
func NewRecoverKey(password, mnemonic string, account, index int) RecoverKey {
return RecoverKey{Password: password, Mnemonic: mnemonic, Account: account, Index: index}
}

// UpdateKeyReq requests updating a key
type UpdateKeyReq struct {
OldPassword string `json:"old_password"`
NewPassword string `json:"new_password"`
}

// NewUpdateKeyReq constructs a new UpdateKeyReq structure.
func NewUpdateKeyReq(old, new string) UpdateKeyReq {
return UpdateKeyReq{OldPassword: old, NewPassword: new}
}

// DeleteKeyReq requests deleting a key
type DeleteKeyReq struct {
Password string `json:"password"`
}

// NewDeleteKeyReq constructs a new DeleteKeyReq structure.
func NewDeleteKeyReq(password string) DeleteKeyReq { return DeleteKeyReq{Password: password} }
Copy link
Contributor

Choose a reason for hiding this comment

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

In go usually anything New* returns a pointer to the struct though, no? Meaning return &DeleteKeyReq{} here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In go usually anything New* returns a pointer to the struct though, no?

I used to believe so. Apparently the use of the New prefix is idiomatic regardless of whether the function returns a struct or a pointer.

4 changes: 3 additions & 1 deletion client/lcd/certificates.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,9 @@ func fingerprintForCertificate(certBytes []byte) (string, error) {
return "", err
}
h := sha256.New()
h.Write(cert.Raw)
if _, err := h.Write(cert.Raw); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: sha256's write actually doesn't err. This is perfect for a nolint with a comment.

See: https://golang.org/src/crypto/sha256/sha256.go?s=4118:4138#L203

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Nice catch, didn't know about that, thanks! Nonetheless, certificates.go will be gone shortly - we intend to remove the --secure option from the REST server altogether.

return "", err
}
fingerprintBytes := h.Sum(nil)
var buf bytes.Buffer
for i, b := range fingerprintBytes {
Expand Down
14 changes: 7 additions & 7 deletions client/lcd/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ func InitializeTestLCD(t *testing.T, nValidators int, initAddrs []sdk.AccAddress
genDoc, err := tmtypes.GenesisDocFromFile(genesisFile)
require.Nil(t, err)
genDoc.Validators = nil
genDoc.SaveAs(genesisFile)
require.NoError(t, genDoc.SaveAs(genesisFile))
genTxs := []json.RawMessage{}

// append any additional (non-proposing) validators
Expand Down Expand Up @@ -337,7 +337,7 @@ func InitializeTestLCD(t *testing.T, nValidators int, initAddrs []sdk.AccAddress

cleanup = func() {
logger.Debug("cleaning up LCD initialization")
node.Stop()
node.Stop() //nolint:errcheck
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still have nolint??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

node.Wait()
lcd.Close()
}
Expand Down Expand Up @@ -394,7 +394,7 @@ func startLCD(logger log.Logger, listenAddr string, cdc *codec.Codec, t *testing
if err != nil {
return nil, err
}
go tmrpc.StartHTTPServer(listener, rs.Mux, logger)
go tmrpc.StartHTTPServer(listener, rs.Mux, logger) //nolint:errcheck
return listener, nil
}

Expand Down Expand Up @@ -559,7 +559,7 @@ func getKeys(t *testing.T, port string) []keys.KeyOutput {

// POST /keys Create a new account locally
func doKeysPost(t *testing.T, port, name, password, mnemonic string, account int, index int) keys.KeyOutput {
pk := clientkeys.AddNewKey{name, password, mnemonic, account, index}
pk := clientkeys.NewAddNewKey(name, password, mnemonic, account, index)
req, err := cdc.MarshalJSON(pk)
require.NoError(t, err)

Expand All @@ -585,7 +585,7 @@ func getKeysSeed(t *testing.T, port string) string {

// POST /keys/{name}/recove Recover a account from a seed
func doRecoverKey(t *testing.T, port, recoverName, recoverPassword, mnemonic string, account uint32, index uint32) {
pk := clientkeys.RecoverKey{recoverPassword, mnemonic, int(account), int(index)}
pk := clientkeys.NewRecoverKey(recoverPassword, mnemonic, int(account), int(index))
req, err := cdc.MarshalJSON(pk)
require.NoError(t, err)

Expand Down Expand Up @@ -613,7 +613,7 @@ func getKey(t *testing.T, port, name string) keys.KeyOutput {

// PUT /keys/{name} Update the password for this account in the KMS
func updateKey(t *testing.T, port, name, oldPassword, newPassword string, fail bool) {
kr := clientkeys.UpdateKeyReq{oldPassword, newPassword}
kr := clientkeys.NewUpdateKeyReq(oldPassword, newPassword)
req, err := cdc.MarshalJSON(kr)
require.NoError(t, err)
keyEndpoint := fmt.Sprintf("/keys/%s", name)
Expand All @@ -627,7 +627,7 @@ func updateKey(t *testing.T, port, name, oldPassword, newPassword string, fail b

// DELETE /keys/{name} Remove an account
func deleteKey(t *testing.T, port, name, password string) {
dk := clientkeys.DeleteKeyReq{password}
dk := clientkeys.NewDeleteKeyReq(password)
req, err := cdc.MarshalJSON(dk)
require.NoError(t, err)
keyEndpoint := fmt.Sprintf("/keys/%s", name)
Expand Down
5 changes: 4 additions & 1 deletion client/rest/rest.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package rest

import (
"log"
"net/http"

"github.com/cosmos/cosmos-sdk/client"
Expand Down Expand Up @@ -67,6 +68,8 @@ func WriteGenerateStdTxResponse(w http.ResponseWriter, cdc *codec.Codec,
}

w.Header().Set("Content-Type", "application/json")
w.Write(output)
if _, err := w.Write(output); err != nil {
log.Printf("could not write response: %v", err)
}
return
}
17 changes: 7 additions & 10 deletions client/rpc/block.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,20 +115,19 @@ func BlockRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
vars := mux.Vars(r)
height, err := strconv.ParseInt(vars["height"], 10, 64)
if err != nil {
w.WriteHeader(http.StatusBadRequest)
w.Write([]byte("ERROR: Couldn't parse block height. Assumed format is '/block/{height}'."))
rest.WriteErrorResponse(w, http.StatusBadRequest,
"ERROR: Couldn't parse block height. Assumed format is '/block/{height}'.")
return
}
chainHeight, err := GetChainHeight(cliCtx)
if height > chainHeight {
w.WriteHeader(http.StatusNotFound)
w.Write([]byte("ERROR: Requested block height is bigger then the chain length."))
rest.WriteErrorResponse(w, http.StatusNotFound,
"ERROR: Requested block height is bigger then the chain length.")
return
}
output, err := getBlock(cliCtx, &height)
if err != nil {
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(err.Error()))
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}
rest.PostProcessResponse(w, cdc, output, cliCtx.Indent)
Expand All @@ -140,14 +139,12 @@ func LatestBlockRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
height, err := GetChainHeight(cliCtx)
if err != nil {
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(err.Error()))
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}
output, err := getBlock(cliCtx, &height)
if err != nil {
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(err.Error()))
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}
rest.PostProcessResponse(w, cdc, output, cliCtx.Indent)
Expand Down
9 changes: 7 additions & 2 deletions client/rpc/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package rpc

import (
"fmt"
"log"
"net/http"

"github.com/gorilla/mux"
Expand All @@ -26,7 +27,9 @@ func RegisterRoutes(cliCtx context.CLIContext, r *mux.Router) {
// cli version REST handler endpoint
func CLIVersionRequestHandler(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
w.Write([]byte(fmt.Sprintf("{\"version\": \"%s\"}", version.Version)))
if _, err := w.Write([]byte(fmt.Sprintf("{\"version\": \"%s\"}", version.Version))); err != nil {
log.Printf("could not write response: %v", err)
}
}

// connected node version REST handler endpoint
Expand All @@ -39,6 +42,8 @@ func NodeVersionRequestHandler(cliCtx context.CLIContext) http.HandlerFunc {
}

w.Header().Set("Content-Type", "application/json")
w.Write(version)
if _, err := w.Write(version); err != nil {
log.Printf("could not write response: %v", err)
}
}
}
Loading