Improve Go Report Card #217

siggy · 2018-01-26T10:21:52Z

We are getting an A+ on Go Report Card, but there is still a lot to improve:
~~https://goreportcard.com/report/github.com/runconduit/conduit~~
https://goreportcard.com/report/github.com/linkerd/linkerd2

We should consider these changes, and also add a badge to README.md.

The text was updated successfully, but these errors were encountered:

klingerf · 2018-01-26T18:07:03Z

Hmm, looks like almost all of the issues in the repo could be found with the golint tool. We should get in the habit of running that.

pcalcado · 2018-01-26T18:10:17Z

Should we create a precommit.sh file or something that does a standardized go test ./... && go vet ./... && golint? Also could be the script travis runs

klingerf · 2018-01-26T18:23:49Z

Yeah, I'd be in favor of auto-running some form of linting, but I'd like to investigate if the linting rules are configurable before we start failing builds. There are a few things turned up by the linter that don't seem important to me, such as:

var mockApiClient should be mockAPIClient

klingerf · 2018-01-26T18:25:27Z

Heh, and this one:

func name will be used as version.VersionFlag by other packages, and that stutters; consider calling this Flag

pcalcado · 2018-01-26T21:45:35Z

I am very easygoing with these things, I can definitely live with the silly ones if it gives us a standardised and well-supported way to check for more interesting things.

rafaeljesus · 2018-02-27T18:19:07Z

IMHO I am not really about skipping these lint errors, it's pretty handy for writing idiomatic golang code ^.^

Also it might be required if you decide to add it at awesome-go list

Part of #217. Follow up from #1982 and #2018. A subsequent commit will fix the ci failure. Signed-off-by: Andrew Seigner <siggy@buoyant.io>

Commit 1: Enable lint check for comments Part of #217. Follow up from #1982 and #2018. A subsequent commit will fix the ci failure. Commit 2: Address all comment-related linter errors. This change addresses all comment-related linter errors by doing the following: - Add comments to exported symbols - Make some exported symbols private - Recommend via TODOs that some exported symbols should should move or be removed This PR does not: - Modify, move, or remove any code - Modify existing comments Signed-off-by: Andrew Seigner <siggy@buoyant.io>

`ineffassign` detects ineffectual assignments in Go code. This change introduces a `bin/ineffassign` into ci, and fixes 4 ineffectual assignments caught by ineffassign. Also perform a one-time gofmt cleanup: - `gofmt -s -w controller/` - `gofmt -s -w pkg/` Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

`golangci-lint` performs numerous checks on Go code, including golint, ineffassign, govet, and gofmt. This change modifies `bin/lint` to use `golangci-lint`, and replaces usage of golint and govet. Also perform a one-time gofmt cleanup: - `gofmt -s -w controller/` - `gofmt -s -w pkg/` Part of #217

`golangci-lint` performs numerous checks on Go code, including golint, ineffassign, govet, and gofmt. This change modifies `bin/lint` to use `golangci-lint`, and replaces usage of golint and govet. Also perform a one-time gofmt cleanup: - `gofmt -s -w controller/` - `gofmt -s -w pkg/` Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy · 2019-02-18T19:39:48Z

Taking this a bit further, #2284 introduced a .golangci.yml file, which specifies which golangci-lint checkers to run.

To help improve the codebase further, we should work towards reducing the number of checkers in the disable list. Essentially repeat this pattern:

remove an item from the disable list
run bin/lint, observe errors
fix errors until bin/lint succeeds

gosimple is a Go linter that specializes in simplifying code Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

gosimple is a Go linter that specializes in simplifying code Also fix one spelling error in `cred_test.go` Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

`unused` checks Go code for unused constants, variables, functions, and types. Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

`varcheck` finds unused global variables and constants. Our repo was already passing so no fixes needed. Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

`deadcode` finds unused code. `varcheck` finds unused global variables and constants. Our repo was already passing both linters so no fixes needed. Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

Enables the following linters: - `deadcode` finds unused code. - `depguard` checks if package imports are in a list of acceptable packages. - `misspell` finds commonly misspelled English words in comments. - `nakedret` finds naked returns in functions greater than a specified function length. - `varcheck` finds unused global variables and constants. Our repo was already passing these linters so no fixes needed. Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

Enables the following linters: - `deadcode` finds unused code. - `depguard` checks if package imports are in a list of acceptable packages. - `misspell` finds commonly misspelled English words in comments. - `nakedret` finds naked returns in functions greater than a specified function length. - `varcheck` finds unused global variables and constants. Our repo was already passing these linters so no fixes needed. Also explicitly list all linters enabled by default, for reference. Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

goimports checks import lines, adding missing ones and removing unreferenced ones: https://godoc.org/golang.org/x/tools/cmd/goimports It also requires named imports for packages whose import paths don't match their package names: - golang/go#28428 - https://go-review.googlesource.com/c/tools/+/145699/ Also standardized named imports of common Kubernetes packaages. Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

goconst finds repeated strings that could be replaced by a constant: https://github.com/jgautheron/goconst Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

interlacer suggests narrower interface types: https://github.com/mvdan/interfacer Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

interfacer suggests narrower interface types: https://github.com/mvdan/interfacer Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

unconvert removes unnecessary type conversions: https://github.com/mdempsky/unconvert Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

unparam reports unused function parameters: https://github.com/mvdan/unparam Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

goconst finds repeated strings that could be replaced by a constant: https://github.com/jgautheron/goconst Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

goimports checks import lines, adding missing ones and removing unreferenced ones: https://godoc.org/golang.org/x/tools/cmd/goimports It also requires named imports for packages whose import paths don't match their package names: - golang/go#28428 - https://go-review.googlesource.com/c/tools/+/145699/ Also standardized named imports of common Kubernetes packaages. Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

unconvert removes unnecessary type conversions: https://github.com/mdempsky/unconvert Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

interfacer suggests narrower interface types: https://github.com/mvdan/interfacer Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

unparam reports unused function parameters: https://github.com/mvdan/unparam Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

interfacer suggests narrower interface types: https://github.com/mvdan/interfacer Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

unparam reports unused function parameters: https://github.com/mvdan/unparam Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

grampelberg · 2019-12-18T23:34:44Z

@siggy this issue doesn't feel particularly actionable at this point (and there's a ton of awesome work that's already been done). WDYT about closing this out and opening up directed issues for what's remaining?

siggy · 2019-12-18T23:37:51Z

@grampelberg I'm fine with closing this one.

Note that we lost our perfect gofmt score recently:
https://goreportcard.com/report/github.com/linkerd/linkerd2#gofmt
This may have happened when we changed our lint config, probably worth a new issue to track this.

grampelberg · 2019-12-18T23:39:03Z

I was wondering about that. Thanks!

siggy added help wanted good first issue labels Jan 26, 2018

radu-matei mentioned this issue Dec 13, 2018

Address linter errors #1982

Merged

siggy added a commit that referenced this issue Dec 22, 2018

Enable lint check for comments

2339700

Part of #217. Follow up from #1982 and #2018. A subsequent commit will fix the ci failure. Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy mentioned this issue Dec 22, 2018

Enable lint check for comments #2023

Merged

siggy mentioned this issue Feb 9, 2019

Introduce golangci-lint tooling, fixes #2239

Merged

siggy added a commit that referenced this issue Feb 23, 2019

Enable gosimple linter, fix issues

d700d33

gosimple is a Go linter that specializes in simplifying code Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added a commit that referenced this issue Feb 23, 2019

Enable gosimple linter, fix issues

b294405

gosimple is a Go linter that specializes in simplifying code Also fix one spelling error in `cred_test.go` Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy mentioned this issue Feb 23, 2019

Enable gosimple linter, fix issues #2356

Merged

siggy added a commit that referenced this issue Feb 23, 2019

Enable gosimple linter, fix issues (#2356)

4b6f6ae

gosimple is a Go linter that specializes in simplifying code Also fix one spelling error in `cred_test.go` Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added a commit that referenced this issue Feb 23, 2019

Enable unused linter

b7c46f5

`unused` checks Go code for unused constants, variables, functions, and types. Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy mentioned this issue Feb 23, 2019

Enable unused linter #2357

Merged

siggy added a commit that referenced this issue Feb 23, 2019

Enable unused linter (#2357)

cc3ff70

`unused` checks Go code for unused constants, variables, functions, and types. Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added a commit that referenced this issue Feb 23, 2019

Enable varcheck linter

9e646fe

`varcheck` finds unused global variables and constants. Our repo was already passing so no fixes needed. Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy mentioned this issue Feb 23, 2019

Enable more linters #2362

Merged

siggy mentioned this issue Feb 24, 2019

lint: Enable goimports #2366

Merged

siggy added a commit that referenced this issue Feb 24, 2019

lint: Enable goconst

9abd59f

goconst finds repeated strings that could be replaced by a constant: https://github.com/jgautheron/goconst Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy mentioned this issue Feb 24, 2019

lint: Enable goconst #2365

Merged

siggy added a commit that referenced this issue Feb 24, 2019

lint: Enable interfacer

f397b74

interlacer suggests narrower interface types: https://github.com/mvdan/interfacer Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added a commit that referenced this issue Feb 24, 2019

lint: Enable interfacer

247bc55

interfacer suggests narrower interface types: https://github.com/mvdan/interfacer Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy mentioned this issue Feb 24, 2019

lint: Enable interfacer #2367

Closed

siggy added a commit that referenced this issue Feb 24, 2019

lint: Enable unconvert

ae56204

unconvert removes unnecessary type conversions: https://github.com/mdempsky/unconvert Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy mentioned this issue Feb 24, 2019

lint: Enable unconvert #2368

Merged

siggy added a commit that referenced this issue Feb 24, 2019

lint: Enable unparam

6d45be8

unparam reports unused function parameters: https://github.com/mvdan/unparam Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy mentioned this issue Feb 24, 2019

lint: Enable unparam #2369

Merged

siggy added a commit that referenced this issue Feb 25, 2019

lint: Enable goconst (#2365)

35a0b65

goconst finds repeated strings that could be replaced by a constant: https://github.com/jgautheron/goconst Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added a commit that referenced this issue Feb 26, 2019

lint: Enable unconvert (#2368)

ad0d0b7

unconvert removes unnecessary type conversions: https://github.com/mdempsky/unconvert Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added a commit that referenced this issue Feb 26, 2019

lint: Enable interfacer

3fc0894

interfacer suggests narrower interface types: https://github.com/mvdan/interfacer Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added a commit that referenced this issue Feb 26, 2019

lint: Enable unparam

c322ff5

unparam reports unused function parameters: https://github.com/mvdan/unparam Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added a commit that referenced this issue Feb 26, 2019

lint: Enable interfacer

d7d3c99

interfacer suggests narrower interface types: https://github.com/mvdan/interfacer Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added a commit that referenced this issue Feb 26, 2019

lint: Enable unparam

f79bd75

unparam reports unused function parameters: https://github.com/mvdan/unparam Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added a commit that referenced this issue Feb 27, 2019

lint: Enable unparam

4d3bc59

unparam reports unused function parameters: https://github.com/mvdan/unparam Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added a commit that referenced this issue Feb 27, 2019

lint: Enable unparam (#2369)

9f748d2

unparam reports unused function parameters: https://github.com/mvdan/unparam Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

grampelberg closed this as completed Dec 18, 2019

github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Go Report Card #217

Improve Go Report Card #217

siggy commented Jan 26, 2018 •

edited

Loading

klingerf commented Jan 26, 2018

pcalcado commented Jan 26, 2018

klingerf commented Jan 26, 2018

klingerf commented Jan 26, 2018

pcalcado commented Jan 26, 2018

rafaeljesus commented Feb 27, 2018

siggy commented Feb 18, 2019

grampelberg commented Dec 18, 2019

siggy commented Dec 18, 2019

grampelberg commented Dec 18, 2019

Improve Go Report Card #217

Improve Go Report Card #217

Comments

siggy commented Jan 26, 2018 • edited Loading

klingerf commented Jan 26, 2018

pcalcado commented Jan 26, 2018

klingerf commented Jan 26, 2018

klingerf commented Jan 26, 2018

pcalcado commented Jan 26, 2018

rafaeljesus commented Feb 27, 2018

siggy commented Feb 18, 2019

grampelberg commented Dec 18, 2019

siggy commented Dec 18, 2019

grampelberg commented Dec 18, 2019

siggy commented Jan 26, 2018 •

edited

Loading