refactor: replace panic with error in rules #1126
Conversation
|
As is, the implementation of rules in this branch does not handle errors but put them "under the carpet" |
# Conflicts: # rule/add_constant.go # rule/argument_limit.go # rule/banned_characters.go # rule/cognitive_complexity.go # rule/comment_spacings.go # rule/comments_density.go # rule/context_as_argument.go # rule/cyclomatic.go # rule/defer.go # rule/dot_imports.go # rule/enforce_map_style.go # rule/enforce_repeated_arg_type_style.go # rule/enforce_slice_style.go # rule/error_strings.go # rule/exported.go # rule/file_header.go # rule/file_length_limit.go # rule/filename_format.go # rule/function_length.go # rule/function_result_limit.go # rule/import_alias_naming.go # rule/imports_blocklist.go # rule/line_length_limit.go # rule/max_control_nesting.go # rule/max_public_structs.go # rule/receiver_naming.go # rule/struct_tag.go # rule/unchecked_type_assertion.go # rule/unhandled_error.go # rule/unused_param.go # rule/unused_receiver.go # rule/var_naming.go
| @@ -14,7 +14,7 @@ type DisabledInterval struct { | |||
| // Rule defines an abstract rule interface | |||
| type Rule interface { | |||
| Name() string | |||
| Apply(*File, Arguments) []Failure | |||
| Apply(*File, Arguments) ([]Failure, error) | |||
There was a problem hiding this comment.
We can't simply add a new return argument to the Apply function due to the backward compatibility. Someone who uses revivelib may depend on this interface.
There was a problem hiding this comment.
Ok @alexandear any idea how to handle errors without of course excuting panic(), we can use in each Apply:
if err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}
but it is not elegant way :(
There was a problem hiding this comment.
To maintain backwards compatibility we would have to add backwards compatible wrappers around the newly included changes. And one thing that came to my mind, how do these changes fit the use of Revive in golangci-lint?
There was a problem hiding this comment.
ok got it @denisvmedia you want something like that: https://github.com/golangci/golangci-lint/blob/master/pkg/golinters/revive/revive.go#L53 ? but is it possible to do that in separate PR, or it should be done in that PR?
lint/linter.go
Outdated
| @@ -152,7 +152,11 @@ func (l *Linter) lintPackage(filenames []string, gover *goversion.Version, ruleS | |||
| return nil | |||
| } | |||
|
|
|||
| pkg.lint(ruleSet, config, failures) | |||
| err := pkg.lint(ruleSet, config, failures) | |||
|
|
|||
There was a problem hiding this comment.
nit: I'd remove this line (it will be more consistent stylistically).
There was a problem hiding this comment.
@denisvmedia maybe something like this: return pkg.lint(ruleSet, config, failures)?
| @@ -153,7 +153,7 @@ func (l *Linter) lintPackage(filenames []string, gover *goversion.Version, ruleS | |||
| } | |||
|
|
|||
| pkg.lint(ruleSet, config, failures) | |||
|
|
|||
| check := sync.OnceValue(func() error {return r.configure(arguments)}) | ||
| if err := check(); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This won't work as you expected. The configure will be called multiple times.
You can check this by adding the statement fmt.Println("configure add-constant") inside the func() error. After running revive with enabled add-constant on a real project with multiple packages, there are a lot of lines configure add-constant.
related with PR #1107
changes in areas:
I am open to suggestion what to change in next steps @chavacava @denisvmedia