gometalinter -> golangci-lint migration #3933

alessio · 2019-03-18T22:19:51Z

{,scripts/}Makefile:
- Remove gometalinter, install golangci-lint.
- Remove distinction between tools and devtools.
  Just tools is enough.
- test_lint -> lint
  Migrating away from underscore separated names.
- Remove unnecessary targets.
- Drop tendermint/lint. Incompatbile with golangci-lint
  and no longer necessary anyway.

Port tools/gometalinter.json to .golangci.yml
Update CircleCI config accordingly.

Closes: #3896
Also addresses #2835

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry: sdkch add [section] [stanza] [message]
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2019-03-18T22:30:27Z

Codecov Report

Merging #3933 into develop will decrease coverage by 0.04%.
The diff coverage is 13.63%.

@@             Coverage Diff             @@
##           develop    #3933      +/-   ##
===========================================
- Coverage    60.26%   60.22%   -0.05%     
===========================================
  Files          196      197       +1     
  Lines        14615    14628      +13     
===========================================
+ Hits          8808     8809       +1     
- Misses        5214     5225      +11     
- Partials       593      594       +1

client/keys/types.go

{,scripts/}Makefile: - Remove gometalinter, install golangci-lint. - Remove distinction between tools and devtools. Just tools is enough. - test_lint -> lint Migrating away from underscore separated names. - Remove unnecessary targets. - Drop tendermint/lint. Incompatbile with golangci-lint and no longer necessary anyway. Port tools/gometalinter.json to .golangci.yml Update CircleCI config accordingly. Closes: #3896

rigelrozanski

couple minor things - thanks for this

rigelrozanski · 2019-03-18T23:42:55Z

client/lcd/test_helpers.go

@@ -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


do we still have nolint??

oh yes, we do https://github.com/golangci/golangci-lint#false-positives

client/rpc/status.go

rigelrozanski · 2019-03-18T23:49:17Z

x/gov/client/cli/query.go

@@ -43,7 +43,7 @@ $ gaiacli query gov proposal 1

 			var proposal gov.Proposal
 			cdc.MustUnmarshalJSON(res, &proposal)
-			return cliCtx.PrintOutput(proposal)
+			return cliCtx.PrintOutput(proposal) // nolint:errcheck


why do we need this errcheck suppression for lines like these? Is there anything we can do to fix this lint issue? what's causing it?

PrintOutput() returns err

Looks like a bug in errcheck to me...

if

if err := cliCtx.PrintOutput(proposal); err != nil { return err } return nil

is not an error but what we have is an error, then yes, it looks like an errcheck bug to me

Co-Authored-By: alessio <[email protected]>

dep -> go mod migration leftover

sabau

👍 simplification is always nice.
Would be great to find a way to remove the lint ignore

Makefile

liamsi

I left a few comments. The PR LGTM (especially that it simplifies the Makefile) 👍

The only thing to reconsider: The file install-golangci.sh should probably be downloaded during setup time instead of copied from the golangci-lint repository: https://github.com/golangci/golangci-lint/blob/master/install.sh (note the License there)

Drop tendermint/lint. Incompatbile with golangci-lint
and no longer necessary anyway.

Question: Does that mean we can delete/archive this repo?

liamsi · 2019-03-19T11:35:26Z

client/keys/types.go

 // 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} }


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

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.

liamsi · 2019-03-19T11:39:12Z

client/lcd/certificates.go

@@ -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 {


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

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.

liamsi · 2019-03-19T11:43:02Z

x/gov/client/cli/query.go

@@ -43,7 +43,7 @@ $ gaiacli query gov proposal 1

 			var proposal gov.Proposal
 			cdc.MustUnmarshalJSON(res, &proposal)
-			return cliCtx.PrintOutput(proposal)
+			return cliCtx.PrintOutput(proposal) // nolint:errcheck


Looks like a bug in errcheck to me...

tools/install-golangci.sh

…ler and verify it

liamsi

👍

cwgoes

ACK; new linter & Makefile scripts all seem to work. One minor comment.

Makefile

This reverts commit b226049.

This reverts commit 471f8c0.

sabau · 2019-03-19T15:36:59Z

.circleci/config.yml

@@ -200,6 +225,9 @@ jobs:
      - checkout
      - *dependencies
      - run: mkdir -p /tmp/logs
+      - restore_cache:


It's a bit too much of restore_cache, should happen only in the required job, in this case setup_dependencies

and when installing/downloading occurs.
I'll double check now to be sure which of those should keep it

setup_dependencies is a job that runs only once. Other jobs need to restore the cache indipendently.

but are all of them using it?

alessio requested review from alexanderbez and jackzampolin March 18, 2019 22:19

alessio requested review from cwgoes, ebuchman and rigelrozanski as code owners March 18, 2019 22:19

alessio requested review from fedekunze, liamsi, sabau, mossid and sunnya97 March 18, 2019 22:32

fedekunze reviewed Mar 18, 2019

View reviewed changes

client/keys/types.go Show resolved Hide resolved

alessio force-pushed the alessio/golangci-lint branch from 6a6289d to 8df582c Compare March 18, 2019 22:51

rigelrozanski approved these changes Mar 18, 2019

View reviewed changes

alessio added the tooling dev tooling within the sdk label Mar 19, 2019

rigelrozanski and others added 3 commits March 19, 2019 03:05

Update client/rpc/status.go

ca376bd

Co-Authored-By: alessio <[email protected]>

couldn't to could not

9c4a074

Fix misleading message in go-mod-cache

877c91a

dep -> go mod migration leftover

sabau approved these changes Mar 19, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

liamsi reviewed Mar 19, 2019

View reviewed changes

alessio added 2 commits March 19, 2019 13:43

Let's not carry install-golangci.sh with us, just download the instal…

367c5a3

…ler and verify it

Update go.sum

bd033d0

liamsi approved these changes Mar 19, 2019

View reviewed changes

Include tools in .PHONY

1a510b5

cwgoes approved these changes Mar 19, 2019

View reviewed changes

Makefile Show resolved Hide resolved

alessio added 4 commits March 19, 2019 14:29

Make lint depend on golangci-lint only

b226049

Fix build

70da636

Revert "Make lint depend on golangci-lint only"

4970e6a

This reverts commit b226049.

What happens if I don't checkout VCS before running lint?

471f8c0

alessio added 3 commits March 19, 2019 15:18

Revert "What happens if I don't checkout VCS before running lint?"

bb80138

This reverts commit 471f8c0.

Make ci-target to avoid download tools twice

add88ab

Attempt to introduce go modules cache

b09c76c

sabau reviewed Mar 19, 2019

View reviewed changes

Run tests with -mod=readonly

0712cde

alessio merged commit cdf2b7a into develop Mar 19, 2019

alessio deleted the alessio/golangci-lint branch March 19, 2019 16:52

dokwon mentioned this pull request Mar 28, 2019

Update gomalinter to golangci-lint terra-money/classic-core#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gometalinter -> golangci-lint migration #3933

gometalinter -> golangci-lint migration #3933

alessio commented Mar 18, 2019 •

edited

Loading

codecov bot commented Mar 18, 2019 •

edited

Loading

rigelrozanski left a comment

rigelrozanski Mar 18, 2019

alessio Mar 19, 2019

rigelrozanski Mar 18, 2019

alessio Mar 19, 2019

liamsi Mar 19, 2019

rigelrozanski Mar 19, 2019

sabau left a comment

liamsi left a comment

liamsi Mar 19, 2019

alessio Mar 19, 2019

liamsi Mar 19, 2019

alessio Mar 19, 2019

liamsi Mar 19, 2019

liamsi left a comment

cwgoes left a comment

sabau Mar 19, 2019

sabau Mar 19, 2019

alessio Mar 19, 2019

sabau Mar 19, 2019

alessio Mar 19, 2019

gometalinter -> golangci-lint migration #3933

gometalinter -> golangci-lint migration #3933

Conversation

alessio commented Mar 18, 2019 • edited Loading

codecov bot commented Mar 18, 2019 • edited Loading

Codecov Report

rigelrozanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabau left a comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio commented Mar 18, 2019 •

edited

Loading

codecov bot commented Mar 18, 2019 •

edited

Loading