Update default rule set to match recommendations #799
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before this change, Revive's defaults did not match the recommended configuration: https://github.com/mgechev/revive#recommended-configuration With this change in place, Revive now defaults to following its own recommendations.
4 tasks
|
Hi @walles, thanks for the PR. Default rules are those allowing The recommended configuration is very opinionated and it might or not fit your needs. ignoreGeneratedHeader = false
severity = "warning"
confidence = 0.8
errorCode = 0
warningCode = 0
[rule.constant-logical-expr]
[rule.time-equal]
[rule.unreachable-code]
[rule.struct-tag]
[rule.modifies-value-receiver]
[rule.bool-literal-in-expr]
[rule.range-val-in-closure]
[rule.range-val-address]
[rule.datarace]
[rule.string-of-int]
[rule.unconditional-recursion]
[rule.identical-branches] |
Let's ask @mgechev! When you took this decision five years ago, Today, Is it time to revisit this backwards compatibility decision? |
mgechev
approved these changes
Mar 14, 2023
|
Makes sense to evolve revive with the deprecation of golint. Thanks for the PR and @chavacava thanks for raising this concern 🙌 |
abhinav
added a commit
to abhinav/revive
that referenced
this pull request
Mar 15, 2023
This fixes a false positive reported by revive on the following:
select {}
This is a way to block the program indefinitely.
It is used in cases like:
- Running a long-running server in a background thread
and blocking `main` from exiting until the application dies.
This is something you might do if your process has
multiple servers/listeners in the same process.
```go
go grpcListenAndServe()
go httpListenAndServe()
go logFlusher()
select {}
```
- In programs compiled to WASM to prevent the application from exiting,
so that the Javascript components may interact with it.
```go
func main() {
js.Global().Set("foo", foo)
select {}
}
```
Without this, one may see an error like,
"Error: Go program has already exited"
As a workaround, these programs can block forever
by receiving from a throwaway channel (`<-make(chan struct{})`),
but `select {}` is still a completely valid way of doing this,
so supporting it makes sense.
The issue was previously reported in mgechev#698 but was closed
because the author was satisfied with a `//nolint` comment.
Now that this rule is part of the default rule set (mgechev#799)
the case for addressing the false positive is stronger.
Resolves mgechev#804
mgechev
pushed a commit
that referenced
this pull request
Mar 16, 2023
This fixes a false positive reported by revive on the following:
select {}
This is a way to block the program indefinitely.
It is used in cases like:
- Running a long-running server in a background thread
and blocking `main` from exiting until the application dies.
This is something you might do if your process has
multiple servers/listeners in the same process.
```go
go grpcListenAndServe()
go httpListenAndServe()
go logFlusher()
select {}
```
- In programs compiled to WASM to prevent the application from exiting,
so that the Javascript components may interact with it.
```go
func main() {
js.Global().Set("foo", foo)
select {}
}
```
Without this, one may see an error like,
"Error: Go program has already exited"
As a workaround, these programs can block forever
by receiving from a throwaway channel (`<-make(chan struct{})`),
but `select {}` is still a completely valid way of doing this,
so supporting it makes sense.
The issue was previously reported in #698 but was closed
because the author was satisfied with a `//nolint` comment.
Now that this rule is part of the default rule set (#799)
the case for addressing the false positive is stronger.
Resolves #804
ncw
added a commit
to rclone/rclone
that referenced
this pull request
Mar 20, 2023
The upstream revive repo changed the default settings for this linter. We use this through golangci-lint. This change meant lots of errors appearing all at once. We should probably fix these in due course, but for the time being this disables those settings. See: mgechev/revive#799
ncw
added a commit
to rclone/rclone
that referenced
this pull request
Mar 20, 2023
The upstream revive repo changed the default settings for this linter. We use this through golangci-lint. This change meant lots of errors appearing all at once. We should probably fix these in due course, but for the time being this disables those settings. See: mgechev/revive#799
4 tasks
rliebz
added a commit
to rliebz/revive
that referenced
this pull request
Jun 23, 2023
The `if-return` rule was originally a golint rule which was removed from their ruleset for being out of scope. Similarly, it was dropped from revive intentionally as a result of mgechev#537. More recently, it was reintroduced into the default ruleset as a result of mgechev#799 due to a discrepancy in documentation without a discussion of whether this rule in particular belonged as a part of that default rule set. While it is no longer a goal of this project to align 100% with the golint defaults, I believe that this rule gives bad advice often enough that it should not be enabled by default. For example, consider the following code: ```go if err := func1(); err != nil { return err } if err := func2(); err != nil { return err } if err := func3(); err != nil { return err } return nil ``` The `if-return` rule considers this a violation of style, and instead suggests the following: ```go if err := func1(); err != nil { return err } if err := func2(); err != nil { return err } return func3() ``` While this is more terse, it has a few shortcomings compared to the original. In particular, it means extending the size of the diff if changing the order of checks, adding logic after the call that currently happens to be last, or choosing to wrap the error. And in that last case, it can make it less obvious that there is an unwrapped error being propagated up the call stack. This in practice has a very similar effect to disabling trailing commas; while it is not necessarily wrong as a style choice, I don't believe it warrants a position as part of the default ruleset here.
rliebz
added a commit
to rliebz/revive
that referenced
this pull request
Jun 23, 2023
The `if-return` rule was originally a golint rule which was removed from their ruleset for being out of scope. Similarly, it was dropped from revive intentionally as a result of mgechev#537. More recently, it was reintroduced into the default ruleset as a result of mgechev#799 due to a discrepancy in documentation without a discussion of whether this rule in particular belonged as a part of that default rule set. While it is no longer a goal of this project to align 100% with the golint defaults, I believe that this rule gives bad advice often enough that it should not be enabled by default. For example, consider the following code: ```go if err := func1(); err != nil { return err } if err := func2(); err != nil { return err } if err := func3(); err != nil { return err } return nil ``` The `if-return` rule considers this a violation of style, and instead suggests the following: ```go if err := func1(); err != nil { return err } if err := func2(); err != nil { return err } return func3() ``` While this is more terse, it has a few shortcomings compared to the original. In particular, it means extending the size of the diff if changing the order of checks, adding logic after the call that currently happens to be last, or choosing to wrap the error. And in that last case, it can make it less obvious that there is an unwrapped error being propagated up the call stack. This in practice has a very similar effect to disabling trailing commas; while it is not necessarily wrong as a style choice, I don't believe it warrants a position as part of the default ruleset here.
rliebz
added a commit
to rliebz/revive
that referenced
this pull request
Jun 23, 2023
The `if-return` rule was originally a golint rule which was removed from their ruleset for being out of scope. Similarly, it was dropped from revive intentionally as a result of mgechev#537. More recently, it was reintroduced into the default ruleset as a result of mgechev#799 due to a discrepancy in documentation without a discussion of whether this rule in particular belonged as a part of that default rule set. While it is no longer a goal of this project to align 100% with the golint defaults, I believe that this rule gives bad advice often enough that it should not be enabled by default. For example, consider the following code: ```go if err := func1(); err != nil { return err } if err := func2(); err != nil { return err } if err := func3(); err != nil { return err } return nil ``` The `if-return` rule considers this a violation of style, and instead suggests the following: ```go if err := func1(); err != nil { return err } if err := func2(); err != nil { return err } return func3() ``` While this is more terse, it has a few shortcomings compared to the original. In particular, it means extending the size of the diff if changing the order of checks, adding logic after the call that currently happens to be last, or choosing to wrap the error. And in that last case, it can make it less obvious that there is an unwrapped error being propagated up the call stack. This in practice has a very similar effect to disabling trailing commas; while it is not necessarily wrong as a style choice, I don't believe it warrants a position as part of the default ruleset here. See-also: golang/lint#363
mgechev
pushed a commit
that referenced
this pull request
Jun 26, 2023
The `if-return` rule was originally a golint rule which was removed from their ruleset for being out of scope. Similarly, it was dropped from revive intentionally as a result of #537. More recently, it was reintroduced into the default ruleset as a result of #799 due to a discrepancy in documentation without a discussion of whether this rule in particular belonged as a part of that default rule set. While it is no longer a goal of this project to align 100% with the golint defaults, I believe that this rule gives bad advice often enough that it should not be enabled by default. For example, consider the following code: ```go if err := func1(); err != nil { return err } if err := func2(); err != nil { return err } if err := func3(); err != nil { return err } return nil ``` The `if-return` rule considers this a violation of style, and instead suggests the following: ```go if err := func1(); err != nil { return err } if err := func2(); err != nil { return err } return func3() ``` While this is more terse, it has a few shortcomings compared to the original. In particular, it means extending the size of the diff if changing the order of checks, adding logic after the call that currently happens to be last, or choosing to wrap the error. And in that last case, it can make it less obvious that there is an unwrapped error being propagated up the call stack. This in practice has a very similar effect to disabling trailing commas; while it is not necessarily wrong as a style choice, I don't believe it warrants a position as part of the default ruleset here. See-also: golang/lint#363
This was referenced Mar 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this change, Revive's defaults did not match the recommended configuration:
https://github.com/mgechev/revive#recommended-configuration
I found that to be confusing.
With this change in place, Revive now defaults to following its own recommendations.
Closes #800
And as a (side?) note, I just fixed actual bugs in my code by enabling unused-parameters warnings. Woho for unused parameters warnings!