-
Notifications
You must be signed in to change notification settings - Fork 800
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Golint is borderline abandonware, and doesn't even ignore vendor folders from `./...`.
Plus it's non-configurable by design, which encourages some rather nasty habits, like useless comments:
```go
// Thing is a thing
type Thing struct { ... }
```
Bad documentation is worse than no documentation, and it wastes space and effort.
After a brief skim of the current state of things, `revive` popped out as a possibility,
and the results are pretty good. Runtimes:
```
# time make lint (before)
make lint 8.80s user 13.14s system 154% cpu 14.206 total
# time make lint (simply swapping bins)
make lint 10.85s user 16.89s system 149% cpu 18.545 total
# time make lint (new call)
make lint 21.72s user 4.22s system 2555% cpu 1.015 total
```
So that's a 14x reduction in runtime, and now it's at the point where it's fine to run all the time.
Golint-like settings finds a few additional things, but nothing major. They are essentially all the same as:
```
# canary is an ignored folder currently, config will skip this
canary/canary.go:139:8: should not use basic type string as key in context.WithValue
# new kind of lint, maybe worth following
common/domain/replication_queue.go:233:2: redundant if ...; err != nil check, just return error instead.
# new kind of lint, likely worth following
service/worker/parentclosepolicy/processor.go:77:9: should not use basic type string as key in context.WithValue
```
The additional "higher value" ones I've added are potentially significant though, and all four have violations currently.
`defer` in particular appears both quite dangerous and definitely has flawed use, though *most* cases are used correctly (the lint has false positives).
Personally I'd like to rewrite the code to avoid these false positives - it is a deeply risky pattern, and absolutely nothing helps you detect incorrect use.
`unhandled-error` is basically `errcheck`, and I'd enable it immediately... but we have almost 300 violations.
IMO these should absolutely be fixed, or carefully suppressed in code we are fine being highly-yolo with. But they currently drown out other lints, so I've commented it out.
Other possibly-useful lints that are not added yet:
- `[rule.bool-literal-in-expr]` finds one, seems fine to prevent
- `[rule.deep-exit]` finds 38, and... may be worth following. Select use in top-level CLI code seems fine, elsewhere is dubious.
- `[rule.duplicated-imports]` surprisingly finds 5.
- `[rule.import-shadowing]` finds... 763 results. To some degree, meh. to some degree, it's probably worth avoiding.
- `[rule.imports-blacklist]` if we have any imports we want to ban (I've had enormous success elsewhere with a check like this)
- `[rule.redefines-builtin-id]` finds two, a `print` and a type `Type`
- `[rule.struct-tag]`, discovers multiple "unknown option 'required' in JSON tag". Is this expected?- Loading branch information
Showing
5 changed files
with
82 additions
and
31 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| # config for https://github.com/mgechev/revive | ||
| ignoreGeneratedHeader = false | ||
| severity = "warning" | ||
| confidence = 0.8 | ||
| errorCode = 0 | ||
| warningCode = 0 | ||
|
|
||
| #### roughly what golint does | ||
|
|
||
| [rule.blank-imports] | ||
| [rule.context-as-argument] | ||
| [rule.context-keys-type] | ||
| [rule.dot-imports] | ||
| [rule.error-naming] | ||
| [rule.error-return] | ||
| [rule.error-strings] | ||
| [rule.errorf] | ||
| # [rule.exported] # comment formatting lint, disabled due to noise / lack of value | ||
| [rule.if-return] | ||
| [rule.increment-decrement] | ||
| [rule.indent-error-flow] | ||
| [rule.package-comments] | ||
| [rule.range] | ||
| [rule.receiver-naming] | ||
| [rule.time-naming] | ||
| [rule.unexported-return] | ||
| [rule.var-declaration] | ||
| [rule.var-naming] | ||
|
|
||
| #### higher value stuff | ||
|
|
||
| # this is basically errcheck, warns on errs that are not checked. | ||
| # strongly desired, but disabled due to excessive failures (to be tackled incrementally). | ||
| # [rule.unhandled-error] | ||
|
|
||
| # warns about unsafe use of recover(), has caught bugs. | ||
| # the arguments are excluding only "call-chain", which would disallow `defer someFn(...)()` which is both useful and in use. | ||
| [rule.defer] | ||
| arguments=[["loop","method-call","recover","return"]] | ||
| # warns about modifying 'self' in a value receiver, which can often be a bug due to implicit copies of the value. | ||
| [rule.modifies-value-receiver] | ||
| # go vet considers this a fatal error, but only in 1.15 or newer, and go.mod currently targets 1.13 | ||
| [rule.string-of-int] | ||
|
|
||
| #### added because we currently have no violations, and they seem decent enough to retain | ||
|
|
||
| [rule.modifies-parameter] | ||
| [rule.unreachable-code] | ||
| [rule.range-val-in-closure] | ||
| [rule.range-val-address] | ||
| [rule.waitgroup-by-value] | ||
| [rule.atomic] | ||
| [rule.call-to-gc] | ||
| [rule.identical-branches] |