Rationalize failure categories

[chavacava]

Failures reported by revive can be of one of 18 categories (code style, bad practice, complexity, ...)
IMO there are too specific categories (like time, imports, content, ...) and that results in too many categories.

I propose to rationalize failure categories, to simplify assigning a failure to a category and leverage category information (for example to provide a way of filtering returned failures by category)

A first proposal with 4 categories:

1. style: all failures related to code style (intrinsically opinionated),
2. bad practice: failures related to coding patterns that are commonly accepted (less opinionated) as "not okay",
3. error: failures that point actual errors,
4. optimization: "failures" that identify memory/runtime optimization opportunities.

The current 18 categories could be organized into the above 4 as follows:

Old	New
FailureCategoryArgOrder	style
FailureCategoryBadPractice	bad practice
FailureCategoryCodeStyle	style
FailureCategoryComments	style
FailureCategoryComplexity	style
FailureCategoryContent	bad practice
FailureCategoryErrors	style
FailureCategoryImports	style
FailureCategoryLogic	error (with some exceptions that can be "bad practice")
FailureCategoryMaintenance	style
FailureCategoryNaming	style
FailureCategoryOptimization	optimization
FailureCategoryStyle	style
FailureCategoryTime	error (time_equal) style(time_naming)
FailureCategoryTypeInference	style
FailureCategoryUnaryOp	style
FailureCategoryUnexportedTypeInAPI	bad practice
FailureCategoryZeroValue	style

[ccoVeille]

I agree. I worked on exported, and I can tell some are stylistic, but some are bad usages

[chavacava]

I've assigned a category to each failure.

Failure	New category
"string literal %s appears, at least, %d times, create a named constant for it"	style
"avoid magic numbers like '%s', create a named constant for it"	style
"maximum number of arguments per function exceeded; max %d but got %d"	style
"direct assignment to atomic value"	bad practice
"banned character found: %s"	style
"avoid using bare returns, please add return expressions"	style
"a blank import should be only in a main or test package, or have a comment justifying it"	style
"Boolean expression seems to always evaluate to "	error
"omit Boolean literal in expression"	style
"explicit call to the garbage collector"	bad practice
"function %s has cognitive complexity %d (> max enabled %d)"	style
"no space between comment delimiter and comment text"	style
"the file has a comment density of %2.f%% (%d comment lines for %d code lines) but expected a minimum of %d%%"	style
"Method '%s' differs only by capitalization to %s '%s' in %s"	bad practice
"Field '%s' differs only by capitalization to other field in the struct type %s"	bad practice
"unnamed results of the same type may be confusing, consider using named results"	style
"expression always evaluates to true"	error
"expression always evaluates to false"	error
"left and right hand-side sub-expressions are the same"	error
"context.Context should be the first parameter of a function"	bad practice (style)
"should not use basic type %s as key in context.WithValue"	bad practice
"function %s has cyclomatic complexity %d (> max enabled %d)"	style
"datarace: range value %s is captured (by-reference) in goroutine"	error
"potential datarace: return value %s is captured (by-reference) in goroutine"	error
"calls to %s.%s only in main() or init() functions"	bad practice
"return in a defer function has no effect"	error
"recover must be called inside a deferred function"	error
"recover must be called inside a deferred function, this is executing recover immediately"	error
"prefer not to defer inside loops"	bad practice
"prefer not to defer chains of function calls"	bad practice
"be careful when deferring calls to methods without pointer receiver"	bad practice
"should not use dot imports"	style
"Package %s already imported"	error
"this block is empty, you can remove it"	style
"extra empty line at the start of a block"	style
"extra empty line at the end of a block"	style
"use make(map[type]type) instead of map[type]type{}"	style
"use map[type]type{} instead of make(map[type]type)"	style
"argument types should not be omitted"	style
"repeated argument type %q can be omitted"	style
"return types should not be omitted"	style
"repeated return type %q can be omitted"	style
"use nil slice declaration (e.g. var args []type) instead of []type{}"	style
"use make([]type) instead of []type{} (or declare nil slice)"	style
"use nil slice declaration (e.g. var args []type) instead of make([]type, 0)"	style
"use []type{} instead of make([]type, 0) (or declare nil slice)"	style
"error var %s should have name of the form %sFoo"	style
"error should be the last type when returning multiple items"	bad practice (style)
"error strings should not be capitalized or end with punctuation or a newline"	bad practice (style)
"should replace %s(fmt.Sprintf(...)) with %s.Errorf(...)"	style
"exported %s %s should have comment or be unexported"	style
`comment on exported %s %s should be of the form "%s..."	style
"%s name will be used as %s.%s by other packages, and that %s; consider calling this %s"	style
"exported type %v should have comment or be unexported"	style
comment on exported type %v should be of the form "%s..." (with optional leading article)	style
"exported %s %s should have its own declaration"	style
"exported %s %s should have comment%s or be unexported"	style
comment on exported %s %s should be of the form "%s..."	style
public interface method %s.%s should be commented"	style (bad practice)
comment on exported interface method %s.%s should be of the form "%s..."	style
"the file doesn't have an appropriate header"	style
"file length is %d lines, which exceeds the limit of %d"	style
"Filename %s is not of the format %s.%s"	style
"parameter '%s' seems to be a control flag, avoid control coupling"	bad practice
"maximum number of statements per function exceeded; max %d but got %d"	style
"maximum number of lines per function exceeded; max %d but got %d"	style
"maximum number of return results per function exceeded; max %d but got %d"	style
"function '%s' seems to be a getter but it does not return any result	bad practice
"both branches of the if are identical"	error
"redundant if ...; err != nil check, just return error instead."	style
"import name (%s) must match the regular expression: %s"	style
"import name (%s) must NOT match the regular expression: %s"	style
"The name '%s' shadows an import name"	bad practice
"should not use the following blocklisted import: "	style
(increment/decrement) "should replace %s with %s%s"	style
"line is %d characters, out of limit %d"	style
"control flow nesting exceeds %d"	style
"you have exceeded the maximum number (%d) of public struct declarations"	style
"parameter '%s' seems to be modified"	bad practice
"suspicious assignment to a by-value method receiver"	bad practice (error)
"no nested structs are allowed"	style
"for better performance '%v' might be rewritten as '%v'"	optimization
"should have a package comment"	style
"package comment is detached; there should be no blank lines between it and the package statement"	style
package comment should be of the form "%s..."	style
"suspicious assignment of '%s'. range-loop variables always have the same address"	bad practice (error)
"loop variable %v captured by func literal"	error
"should omit 2nd value from range; this loop is equivalent to for %s %s range ..."	style
"receiver name should not be an underscore, omit the name if it is unused"	style
receiver name should be a reflection of its identity; don't use generic names such as "this" or "self"	style
"receiver name %s is longer than %d characters"	style
"receiver name %s should be consistent with previous receiver name %s for %s"	bad practice (style)
The build tag "// +build" is redundant since Go 1.17 and can be removed	(simplification?)
"Import alias "%s" is redundant"	(simplification?)
"redundant call to %s.%s in TestMain function, the test runner will handle it automatically as of Go 1.15"	(simplification?)
(string format failures)	style
"dubious conversion of an integer into a string, use strconv.Itoa"	bad practice
(struct-tag failures)	error
(time) "use %s%s.Equal(%s) instead of %q operator"	bad practice
"var %s is of type %v; don't use unit-specific suffix %q"	style
"type assertion will panic if not matched"	error (bad practice)
"type assertion result ignored"	bad practice
"unconditional recursive call"	error
"the symbol %s is local, its name should start with a lowercase letter"	style
"exported %s %s returns unexported type %s, which can be annoying to use"	bad practice
"Unhandled error in call to function %v"	bad practice
"omit unnecessary return statement"	(simplification?)
"omit unnecessary break at the end of case clause"	(simplification?)
"switch with only one case can be replaced by an if-then"	style (simplification?)
"unreachable code after this statement"	error
(unused-param)	(simplification?)
(unused-receiver)	(simplification?)
"since Go 1.18 'interface{}' can be replaced by 'any'"	style (simplification?)
"replace fmt.Errorf by errors.New"	optimization
"useless break in case clause"	(simplification?)
"should drop = %s from declaration of var %s; it is the zero value"	simplification
"should omit type %s from declaration of var %s; it will be inferred from the right-hand side"	simplification
"don't use an underscore in package name"	style
"don't use MixedCaps in package name; %s should be %s"	style
"don't use ALL_CAPS in Go names; use CamelCase"	style
"don't use underscores in Go names; %s %s should be %s"	style
(var-naming) "%s %s should be %s"	style
"sync.WaitGroup passed by value, the function will get a copy of the original one"	bad practice

a new category simplification emerges.
style could be refined into code conventions and GO idiomatic

[denisvmedia]

I really like your research and proposal.