Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: replace panic with error in rules #1126

Merged
merged 44 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from 41 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
89322bc
refactor: replace panic with error in rules
mfederowicz Nov 14, 2024
5665063
handle outside apply
mfederowicz Nov 16, 2024
cf72c3d
handle error from r.configure
mfederowicz Nov 16, 2024
689e3c9
checkNumberOfArguments: handle error
mfederowicz Nov 16, 2024
3109bd6
rules after review cleanup
mfederowicz Nov 16, 2024
c68d908
package.go: revert to master version
mfederowicz Nov 16, 2024
b607ec6
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Nov 16, 2024
f5e3a0e
fix lack of variables after merge with master
mfederowicz Nov 16, 2024
5c98306
handle configureErr in Apply func
mfederowicz Nov 17, 2024
63a0088
replace configureErr with local variable
mfederowicz Nov 17, 2024
c7f09e0
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Nov 18, 2024
a53dfc7
Merge remote-tracking branch 'upstream/master' into rules-avoid-using…
mfederowicz Nov 18, 2024
d47112b
remove package comment, except doc.go
mfederowicz Nov 18, 2024
fd97ee3
revert new line above:configureOnce
mfederowicz Nov 18, 2024
a451c02
cleanup code
mfederowicz Nov 19, 2024
1c6e8e7
cleanup code
mfederowicz Nov 19, 2024
f17c9b1
replace configureOnce with sync.OnceValue
mfederowicz Nov 20, 2024
ae29706
lint: remove error return, exit is last step
mfederowicz Nov 20, 2024
fdcaa68
back to r.configureOnce.Do() variant with local error variable
mfederowicz Dec 7, 2024
af86a92
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Dec 7, 2024
0022083
cleanup after merge with master
mfederowicz Dec 7, 2024
9c9e05e
package: handle errgroup.Group
mfederowicz Dec 7, 2024
92bcf9e
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Dec 7, 2024
aee44cb
Merge remote-tracking branch 'upstream/master' into rules-avoid-using…
mfederowicz Dec 8, 2024
2d6cafa
after review changes
mfederowicz Dec 8, 2024
ea832f4
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Dec 8, 2024
d6f17ff
cleanup rules, remove error from return signature
mfederowicz Dec 8, 2024
3086a32
cleanup lint/rule.go
mfederowicz Dec 8, 2024
093971c
error msg lowercase
mfederowicz Dec 8, 2024
d78563b
lowercace error msg
mfederowicz Dec 8, 2024
5685fad
.gitignore: add .idea dir
mfederowicz Dec 8, 2024
68e556a
merge with upstream
mfederowicz Dec 8, 2024
3191797
merge with upstream
mfederowicz Dec 8, 2024
f6c7ee0
lowercase in error msg
mfederowicz Dec 8, 2024
0e4a8c4
fix rule name
mfederowicz Dec 8, 2024
ef674f6
helper: newInternalFailureError
mfederowicz Dec 9, 2024
5ebe459
add space in msg
mfederowicz Dec 9, 2024
953b2a5
newInternalFailureError: update helper comment
mfederowicz Dec 9, 2024
ed5e149
.gitignore: remove idea entry
mfederowicz Dec 9, 2024
1628b5c
cleanup after review
mfederowicz Dec 10, 2024
96de931
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Dec 10, 2024
bad6ed8
cleanup after review
mfederowicz Dec 10, 2024
634fa48
Apply suggestions from code review
chavacava Dec 11, 2024
e56fa62
fix printf parameters
Dec 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lint/linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func (l *Linter) Lint(packages [][]string, ruleSet []Rule, config Config) (<-cha
for n := range packages {
go func(pkg []string, gover *goversion.Version) {
if err := l.lintPackage(pkg, gover, ruleSet, config, failures); err != nil {
fmt.Fprintln(os.Stderr, err)
fmt.Fprintln(os.Stderr, "error during linting: "+err.Error())
os.Exit(1)
}
wg.Done()
Expand Down
15 changes: 8 additions & 7 deletions rule/add_constant.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package rule

import (
"errors"
"fmt"
"go/ast"
"regexp"
Expand Down Expand Up @@ -45,7 +46,7 @@ func (r *AddConstantRule) Apply(file *lint.File, arguments lint.Arguments) []lin
var configureErr error
r.configureOnce.Do(func() { configureErr = r.configure(arguments) })
if configureErr != nil {
return []lint.Failure{lint.NewInternalFailure(configureErr.Error())}
return newInternalFailureError(configureErr)
}

var failures []lint.Failure
Expand Down Expand Up @@ -232,35 +233,35 @@ func (r *AddConstantRule) configure(arguments lint.Arguments) error {
}
list, ok := v.(string)
if !ok {
fmt.Errorf("invalid argument to the add-constant rule, string expected. Got '%v' (%T)", v, v)
return fmt.Errorf("invalid argument to the add-constant rule, string expected. Got '%v' (%T)", v, v)
}
r.allowList.add(kind, list)
case "maxLitCount":
sl, ok := v.(string)
if !ok {
fmt.Errorf("invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v' (%T)", v, v)
return fmt.Errorf("invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v' (%T)", v, v)
}

limit, err := strconv.Atoi(sl)
if err != nil {
fmt.Errorf("invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v'", v)
return fmt.Errorf("invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v'", v)
}
r.strLitLimit = limit
case "ignoreFuncs":
excludes, ok := v.(string)
if !ok {
fmt.Errorf("invalid argument to the ignoreFuncs parameter of add-constant rule, string expected. Got '%v' (%T)", v, v)
return fmt.Errorf("invalid argument to the ignoreFuncs parameter of add-constant rule, string expected. Got '%v' (%T)", v, v)
}

for _, exclude := range strings.Split(excludes, ",") {
exclude = strings.Trim(exclude, " ")
if exclude == "" {
fmt.Errorf("invalid argument to the ignoreFuncs parameter of add-constant rule, expected regular expression must not be empty.")
return errors.New("invalid argument to the ignoreFuncs parameter of add-constant rule, expected regular expression must not be empty")
}

exp, err := regexp.Compile(exclude)
if err != nil {
fmt.Errorf("invalid argument to the ignoreFuncs parameter of add-constant rule: regexp %q does not compile: %v", exclude, err)
return fmt.Errorf("invalid argument to the ignoreFuncs parameter of add-constant rule: regexp %q does not compile: %w", exclude, err)
}

r.ignoreFunctions = append(r.ignoreFunctions, exp)
Expand Down
15 changes: 11 additions & 4 deletions rule/argument_limit.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package rule

import (
"errors"
"fmt"
"go/ast"
"sync"
Expand All @@ -17,22 +18,28 @@ type ArgumentsLimitRule struct {

const defaultArgumentsLimit = 8

func (r *ArgumentsLimitRule) configure(arguments lint.Arguments) {
func (r *ArgumentsLimitRule) configure(arguments lint.Arguments) error {
if len(arguments) < 1 {
r.max = defaultArgumentsLimit
return
return nil
}

maxArguments, ok := arguments[0].(int64) // Alt. non panicking version
if !ok {
panic(`invalid value passed as argument number to the "argument-limit" rule`)
return errors.New(`invalid value passed as argument number to the "argument-limit" rule`)
}
r.max = int(maxArguments)
return nil
}

// Apply applies the rule to given file.
func (r *ArgumentsLimitRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
r.configureOnce.Do(func() { r.configure(arguments) })
var configureErr error
r.configureOnce.Do(func() { configureErr = r.configure(arguments) })

if configureErr != nil {
return newInternalFailureError(configureErr)
}

var failures []lint.Failure

Expand Down
28 changes: 21 additions & 7 deletions rule/banned_characters.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,30 @@ type BannedCharsRule struct {

const bannedCharsRuleName = "banned-characters"

func (r *BannedCharsRule) configure(arguments lint.Arguments) {
func (r *BannedCharsRule) configure(arguments lint.Arguments) error {
if len(arguments) > 0 {
checkNumberOfArguments(1, arguments, bannedCharsRuleName)
r.bannedCharList = r.getBannedCharsList(arguments)
err := checkNumberOfArguments(1, arguments, bannedCharsRuleName)
if err != nil {
return err
}
list, err := r.getBannedCharsList(arguments)
if err != nil {
return err
}

r.bannedCharList = list
}
return nil
}

// Apply applied the rule to the given file.
func (r *BannedCharsRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
r.configureOnce.Do(func() { r.configure(arguments) })
var configureErr error
r.configureOnce.Do(func() { configureErr = r.configure(arguments) })

if configureErr != nil {
return newInternalFailureError(configureErr)
}
Comment on lines +39 to +44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose adding a test to check a situation where configureErr is not nil. Not for all rules but at least for a one.


var failures []lint.Failure
onFailure := func(failure lint.Failure) {
Expand All @@ -49,17 +63,17 @@ func (*BannedCharsRule) Name() string {
}

// getBannedCharsList converts arguments into the banned characters list
func (r *BannedCharsRule) getBannedCharsList(args lint.Arguments) []string {
func (r *BannedCharsRule) getBannedCharsList(args lint.Arguments) ([]string, error) {
var bannedChars []string
for _, char := range args {
charStr, ok := char.(string)
if !ok {
panic(fmt.Sprintf("Invalid argument for the %s rule: expecting a string, got %T", r.Name(), char))
return nil, fmt.Errorf("invalid argument for the %s rule: expecting a string, got %T", r.Name(), char)
}
bannedChars = append(bannedChars, charStr)
}

return bannedChars
return bannedChars, nil
}

type lintBannedCharsRule struct {
Expand Down
14 changes: 10 additions & 4 deletions rule/cognitive_complexity.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,29 @@ type CognitiveComplexityRule struct {

const defaultMaxCognitiveComplexity = 7

func (r *CognitiveComplexityRule) configure(arguments lint.Arguments) {
func (r *CognitiveComplexityRule) configure(arguments lint.Arguments) error {
if len(arguments) < 1 {
r.maxComplexity = defaultMaxCognitiveComplexity
return
return nil
}

complexity, ok := arguments[0].(int64)
if !ok {
panic(fmt.Sprintf("invalid argument type for cognitive-complexity, expected int64, got %T", arguments[0]))
return fmt.Errorf("invalid argument type for cognitive-complexity, expected int64, got %T", arguments[0])
}

r.maxComplexity = int(complexity)
return nil
}

// Apply applies the rule to given file.
func (r *CognitiveComplexityRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
r.configureOnce.Do(func() { r.configure(arguments) })
var configureErr error
r.configureOnce.Do(func() { configureErr = r.configure(arguments) })

if configureErr != nil {
return newInternalFailureError(configureErr)
}

var failures []lint.Failure

Expand Down
14 changes: 10 additions & 4 deletions rule/comment_spacings.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,26 @@ type CommentSpacingsRule struct {
configureOnce sync.Once
}

func (r *CommentSpacingsRule) configure(arguments lint.Arguments) {
func (r *CommentSpacingsRule) configure(arguments lint.Arguments) error {
r.allowList = []string{}
for _, arg := range arguments {
allow, ok := arg.(string) // Alt. non panicking version
if !ok {
panic(fmt.Sprintf("invalid argument %v for %s; expected string but got %T", arg, r.Name(), arg))
return fmt.Errorf("invalid argument %v for %s; expected string but got %T", arg, r.Name(), arg)
}
r.allowList = append(r.allowList, `//`+allow)
}
return nil
}

// Apply the rule.
func (r *CommentSpacingsRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure {
r.configureOnce.Do(func() { r.configure(args) })
func (r *CommentSpacingsRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
var configureErr error
r.configureOnce.Do(func() { configureErr = r.configure(arguments) })

if configureErr != nil {
return newInternalFailureError(configureErr)
}

var failures []lint.Failure

Expand Down
14 changes: 10 additions & 4 deletions rule/comments_density.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,29 @@ type CommentsDensityRule struct {

const defaultMinimumCommentsPercentage = 0

func (r *CommentsDensityRule) configure(arguments lint.Arguments) {
func (r *CommentsDensityRule) configure(arguments lint.Arguments) error {
if len(arguments) < 1 {
r.minimumCommentsDensity = defaultMinimumCommentsPercentage
return
return nil
}

var ok bool
r.minimumCommentsDensity, ok = arguments[0].(int64)
if !ok {
panic(fmt.Sprintf("invalid argument for %q rule: argument should be an int, got %T", r.Name(), arguments[0]))
return fmt.Errorf("invalid argument for %q rule: argument should be an int, got %T", r.Name(), arguments[0])
}
return nil
}

// Apply applies the rule to given file.
func (r *CommentsDensityRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
r.configureOnce.Do(func() { r.configure(arguments) })
var configureErr error
r.configureOnce.Do(func() { configureErr = r.configure(arguments) })

if configureErr != nil {
return newInternalFailureError(configureErr)
}

commentsLines := countDocLines(file.AST.Comments)
statementsCount := countStatements(file.AST)
density := (float32(commentsLines) / float32(statementsCount+commentsLines)) * 100
Expand Down
28 changes: 19 additions & 9 deletions rule/context_as_argument.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,13 @@ type ContextAsArgumentRule struct {
}

// Apply applies the rule to given file.
func (r *ContextAsArgumentRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure {
r.configureOnce.Do(func() { r.configure(args) })
func (r *ContextAsArgumentRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
var configureErr error
r.configureOnce.Do(func() { configureErr = r.configure(arguments) })

if configureErr != nil {
return newInternalFailureError(configureErr)
}

var failures []lint.Failure
for _, decl := range file.AST.Decls {
Expand Down Expand Up @@ -59,27 +64,32 @@ func (*ContextAsArgumentRule) Name() string {
return "context-as-argument"
}

func (r *ContextAsArgumentRule) configure(arguments lint.Arguments) {
r.allowTypes = r.getAllowTypesFromArguments(arguments)
func (r *ContextAsArgumentRule) configure(arguments lint.Arguments) error {
types, err := r.getAllowTypesFromArguments(arguments)
if err != nil {
return err
}
r.allowTypes = types
return nil
}

func (r *ContextAsArgumentRule) getAllowTypesFromArguments(args lint.Arguments) map[string]struct{} {
func (r *ContextAsArgumentRule) getAllowTypesFromArguments(args lint.Arguments) (map[string]struct{}, error) {
allowTypesBefore := []string{}
if len(args) >= 1 {
argKV, ok := args[0].(map[string]any)
if !ok {
panic(fmt.Sprintf("Invalid argument to the context-as-argument rule. Expecting a k,v map, got %T", args[0]))
return nil, fmt.Errorf("invalid argument to the context-as-argument rule. Expecting a k,v map, got %T", args[0])
}
for k, v := range argKV {
switch k {
case "allowTypesBefore":
typesBefore, ok := v.(string)
if !ok {
panic(fmt.Sprintf("Invalid argument to the context-as-argument.allowTypesBefore rule. Expecting a string, got %T", v))
return nil, fmt.Errorf("invalid argument to the context-as-argument.allowTypesBefore rule. Expecting a string, got %T", v)
}
allowTypesBefore = append(allowTypesBefore, strings.Split(typesBefore, ",")...)
default:
panic(fmt.Sprintf("Invalid argument to the context-as-argument rule. Unrecognized key %s", k))
return nil, fmt.Errorf("invalid argument to the context-as-argument rule. Unrecognized key %s", k)
}
}
}
Expand All @@ -90,5 +100,5 @@ func (r *ContextAsArgumentRule) getAllowTypesFromArguments(args lint.Arguments)
}

result["context.Context"] = struct{}{} // context.Context is always allowed before another context.Context
return result
return result, nil
}
14 changes: 10 additions & 4 deletions rule/cyclomatic.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,28 @@ type CyclomaticRule struct {

const defaultMaxCyclomaticComplexity = 10

func (r *CyclomaticRule) configure(arguments lint.Arguments) {
func (r *CyclomaticRule) configure(arguments lint.Arguments) error {
if len(arguments) < 1 {
r.maxComplexity = defaultMaxCyclomaticComplexity
return
return nil
}

complexity, ok := arguments[0].(int64) // Alt. non panicking version
if !ok {
panic(fmt.Sprintf("invalid argument for cyclomatic complexity; expected int but got %T", arguments[0]))
return fmt.Errorf("invalid argument for cyclomatic complexity; expected int but got %T", arguments[0])
}
r.maxComplexity = int(complexity)
return nil
}

// Apply applies the rule to given file.
func (r *CyclomaticRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
r.configureOnce.Do(func() { r.configure(arguments) })
var configureErr error
r.configureOnce.Do(func() { configureErr = r.configure(arguments) })

if configureErr != nil {
return newInternalFailureError(configureErr)
}

var failures []lint.Failure
for _, decl := range file.AST.Decls {
Expand Down
Loading
Loading