Use already enabled revive linter and add depguard

[MrAlias]

The revive linter is enabled for golangci-lint. However, the default configuration used for the linter is to disable all checks. This enables some of those disabled checks. In addition this enables the use of the depguard linter instead of using the imports-blacklist rule from revive.

This enables a subset of all revive rules. It tries to avoid opinionated rules that the project currently does not adhere to. All lint rules are successfully being validated against the updated code base.

Enabled rules

blank-imports

Blank import should be only in a main or test package, or have a comment justifying it.

bool-literal-in-expr

Using Boolean literals (true, false) in logic expressions may make the code less readable. This rule suggests removing Boolean literals from logic expressions.

constant-logical-expr

The rule spots logical expressions that evaluate always to the same value.

context-as-argument

By convention, context.Context should be the first parameter of a function. This rule spots function declarations that do not follow the convention.

context-keys-type

Basic types should not be used as a key in context.WithValue.

deep-exit

Packages exposing functions that can stop program execution by exiting are hard to reuse. This rule looks for program exits in functions other than main() or init().

defer

This rule warns on some common mistakes when using defer statement.

These gotchas are described here

dot-imports

Importing with . makes the programs much harder to understand because it is unclear whether names belong to the current package or to an imported package.

More information here

duplicated-imports

It is possible to unintentionally import the same package twice. This rule looks for packages that are imported two or more times.

early-return

In Go it is idiomatic to minimize nesting statements, a typical example is to avoid if-then-else constructions. This rule spots constructions like

if cond {
  // do something
} else {
  // do other thing
  return ...
}

that can be rewritten into more idiomatic:

if ! cond {
  // do other thing
  return ...
}

// do something

empty-block

Empty blocks make code less readable and could be a symptom of a bug or unfinished refactoring.

empty-lines

Sometimes gofmt is not enough to enforce a common formatting of a code-base; this rule warns when there are heading or trailing newlines in code blocks.

error-naming

By convention, for the sake of readability, variables of type error must be named with the prefix err.

error-return

By convention, for the sake of readability, the errors should be last in the list of returned values by a function.

error-strings

By convention, for better readability, error messages should not be capitalized or end with punctuation or a newline.

More information here

errorf

It is possible to get a simpler program by replacing errors.New(fmt.Sprintf()) with fmt.Errorf(). This rule spots that kind of simplification opportunities.

exported

Exported function and methods should have comments. This warns on undocumented exported functions and methods.

More information here

flag-parameter

If a function controls the flow of another by passing it information on what to do, both functions are said to be control-coupled. Coupling among functions must be minimized for better maintainability of the code. This rule warns on boolean parameters that create a control coupling.

identical-branches

An if-then-else conditional with identical implementations in both branches is an error.

if-return

Checking if an error is nil to just after return the error or nil is redundant.

increment-decrement

By convention, for better readability, incrementing an integer variable by 1 is recommended to be done using the ++ operator. This rule spots expressions like i += 1 and i -= 1 and proposes to change them into i++ and i--.

indent-error-flow

To improve the readability of code, it is recommended to reduce the indentation as much as possible. This rule highlights redundant else-blocks that can be eliminated from the code.

More information here

import-shadowing

In Go it is possible to declare identifiers (packages, structs, interfaces, parameters, receivers, variables, constants...) that conflict with the name of an imported package. This rule spots identifiers that shadow an import.

package-comments

Packages should have comments. This rule warns on undocumented packages and when packages comments are detached to the package keyword.

More information here

range

This rule suggests a shorter way of writing ranges that do not use the second value.

range-val-in-closure

Range variables in a loop are reused at each iteration; therefore a goroutine created in a loop will point to the range variable with from the upper scope. This way, the goroutine could use the variable with an undesired value. This rule warns when a range value (or index) is used inside a closure

range-val-address

Range variables in a loop are reused at each iteration. This rule warns when assigning the address of the variable, passing the address to append() or using it in a map.

redefines-builtin-id

Constant names like false, true, nil, function names like append, make, and basic type names like bool, and byte are not reserved words of the language; therefore the can be redefined. Even if possible, redefining these built in names can lead to bugs very difficult to detect.

string-format

This rule allows you to configure a list of regular expressions that string literals in certain function calls are checked against. This is geared towards user facing applications where string literals are often used for messages that will be presented to users, so it may be desirable to enforce consistent formatting.

struct-tag

Struct tags are not checked at compile time. This rule, checks and warns if it finds errors in common struct tags types like: asn1, default, json, protobuf, xml, yaml.

superfluous-else

To improve the readability of code, it is recommended to reduce the indentation as much as possible. This rule highlights redundant else-blocks that can be eliminated from the code.

time-equal

This rule warns when using == and != for equality check time.Time and suggest to time.time.Equal method, for about information follow this link

var-naming

This rule warns when initialism, variable or package naming conventions are not followed.

var-declaration

This rule proposes simplifications of variable declarations.

unconditional-recursion

Unconditional recursive calls will produce infinite recursion, thus program stack overflow. This rule detects and warns about unconditional (direct) recursive calls.

unexported-return

This rule warns when an exported function or method returns a value of an un-exported type.

unhandled-error

This rule warns when errors returned by a function are not explicitly handled on the caller side.

unnecessary-stmt

This rule suggests to remove redundant statements like a break at the end of a case block, for improving the code's readability.

useless-break

This rule warns on useless break statements in case clauses of switch and select statements. Go, unlike other programming languages like C, only executes statements of the selected case while ignoring the subsequent case clauses. Therefore, inserting a break at the end of a case clause has no effect.

Because break statements are rarely used in case clauses, when switch or select statements are inside a for-loop, the programmer might wrongly assume that a break in a case clause will take the control out of the loop. The rule emits a specific warning for such cases.

waitgroup-by-value

Function parameters that are passed by value, are in fact a copy of the original argument. Passing a copy of a sync.WaitGroup is usually not what the developer wants to do.
This rule warns when a sync.WaitGroup expected as a by-value parameter in a function or method.

[MadVikingGod]

Thank you for doing the hard work of fixing all of these linter issues.

For the record, I have 0 opinions of which linters should be on, go fmt is fine by me, but I will accept any linter with three rules.

1. You have to be able to run it locally, and it should be 1 command to check everything.
2. It can't be flaky just like a test. If the false-positive rate goes high let's chuck the linter.
3. We don't spend a lot of time trying to decide what linters to use. I would suggest we have some backoff time for adding like we can reject adding anything outright for 6 months, and use rule 2 to remove. After that backoff time, we can revisit but I don't want to choose paint colors very often.

[pellared]

👍