Skip to content

Commit

Permalink
Document disabled lints, especially unhandled-error
Browse files Browse the repository at this point in the history
With this, `revive.toml` mentions all currently-available lints.

Some seem desirable, but they have more lint failures than I feel like tackling at the moment :)
So they're just documented for now.

---

`unhandled-error` I'd *love* to enable immediately, but ~300 warnings is far too many.
Newly-introduced violations of other lints would just be lost in the noise.

Reducing (or rarely whitelisting) these seems necessary before enabling, which IMO should be considered fairly high priority.
Missing error handling is pretty frequently a source of hard-to-identify misbehavior, and a lint like this is the only feasible way to catch many cases.
It's just far too hard to reliably catch during code review (as evidence: 300 cases!), especially when the returned value(s) are not assigned to anything.
  • Loading branch information
Groxx authored Feb 1, 2021
1 parent 16dd4c7 commit 12acdaf
Showing 1 changed file with 41 additions and 0 deletions.
41 changes: 41 additions & 0 deletions revive.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ warningCode = 0

#### higher value stuff

# this is basically errcheck, warns on errs that are not checked.
# strongly desired, but disabled due to 300 failures (to be tackled incrementally).
# [rule.unhandled-error]

# general defer gotchas.
#
# in particular: "recover" warns about unsafe use of recover().
Expand All @@ -55,3 +59,40 @@ arguments=[["loop","method-call","recover","return"]]
[rule.unconditional-recursion] # probably a good idea
[rule.unreachable-code] # code simplifier
[rule.waitgroup-by-value] # correct use of sync code, important

#### unused utilities

# [rule.file-header] # could possibly replace `copyright -verifyOnly`?
# [rule.imports-blacklist] # simple way to ban imports / enforce wrappers, likely useful

#### disabled but maybe desirable

# [rule.bare-return] # probably beneficial as it's slightly error-prone, but 2,000 failures
# [rule.bool-literal-in-expr] # minor code simplifier, few failures
# [rule.confusing-results] # maybe beneficial, only a few failures
# [rule.deep-exit] # probably a good idea in most code, some failures, but not trivial to adopt
# [rule.duplicated-imports] # minor, but may be worthwhile. failures are weird but harmless
# [rule.early-return] # minor code simplifier, a handful of failures
# [rule.get-return] # existing failures are intentional + desirable, but in principle it's a fine idea
# [rule.import-shadowing] # probably beneficial, but 750 failures
# [rule.redefines-builtin-id] # probably beneficial, few failures
# [rule.struct-tag] # probably beneficial, a few failures
# [rule.superfluous-else] # minor code simplifier, a few failures
# [rule.unexported-naming] # probably beneficial, but 300 failures
# [rule.unused-parameter] # minor code simplifier / clarifier, but 250 failures
# [rule.unused-receiver] # minor code simplifier / clarifier, but 500 failures

#### probably undesirable

# [rule.add-constant] # extremely noisy. 18,000 failures, overwhelmingly for tests or 0/1 which seem totally fine
# [rule.argument-limit] # too arbitrary
# [rule.cognitive-complexity] # dubious value, but possibly interesting
# [rule.confusing-naming] # dubious value, ~50 failures
# [rule.cyclomatic] # dubious value, but possibly interesting
# [rule.empty-block] # easily noticed in code review, but also warns on documented no-op branches, which seem fine
# [rule.empty-lines] # low value, many failures
# [rule.flag-parameter] # interesting, but very noisy
# [rule.function-result-limit] # too arbitrary, easily noticed in code review
# [rule.line-length-limit] # too arbitrary
# [rule.max-public-structs] # too arbitrary
# [rule.unnecessary-stmt] # dubious value

0 comments on commit 12acdaf

Please sign in to comment.