Skip to content

Commit b27780b

Browse files
committed
Drop if-return from default ruleset
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
1 parent e5d5d09 commit b27780b

4 files changed

Lines changed: 4 additions & 6 deletions

File tree

README.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -407,7 +407,6 @@ warningCode = 0
407407
[rule.error-strings]
408408
[rule.error-naming]
409409
[rule.exported]
410-
[rule.if-return]
411410
[rule.increment-decrement]
412411
[rule.var-naming]
413412
[rule.var-declaration]

config/config.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ var defaultRules = []lint.Rule{
3131
&rule.TimeNamingRule{},
3232
&rule.ContextKeysType{},
3333
&rule.ContextAsArgumentRule{},
34-
&rule.IfReturnRule{},
3534
&rule.EmptyBlockRule{},
3635
&rule.SuperfluousElseRule{},
3736
&rule.UnusedParamRule{},
@@ -87,6 +86,7 @@ var allRules = append([]lint.Rule{
8786
&rule.UseAnyRule{},
8887
&rule.DataRaceRule{},
8988
&rule.CommentSpacingsRule{},
89+
&rule.IfReturnRule{},
9090
}, defaultRules...)
9191

9292
var allFormatters = []lint.Formatter{

defaults.toml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ warningCode = 0
1212
[rule.error-strings]
1313
[rule.error-naming]
1414
[rule.exported]
15-
[rule.if-return]
1615
[rule.increment-decrement]
1716
[rule.var-naming]
1817
[rule.var-declaration]

revivelib/core_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"github.com/mgechev/revive/config"
88
"github.com/mgechev/revive/lint"
99
"github.com/mgechev/revive/revivelib"
10+
"github.com/mgechev/revive/rule"
1011
)
1112

1213
func TestReviveLint(t *testing.T) {
@@ -45,7 +46,6 @@ func TestReviveFormat(t *testing.T) {
4546

4647
// ACT
4748
failures, exitCode, err := revive.Format("stylish", failuresChan)
48-
4949
// ASSERT
5050
if err != nil {
5151
t.Fatal(err)
@@ -70,8 +70,7 @@ func TestReviveFormat(t *testing.T) {
7070
}
7171
}
7272

73-
type mockRule struct {
74-
}
73+
type mockRule struct{}
7574

7675
func (r *mockRule) Name() string {
7776
return "mock-rule"
@@ -93,6 +92,7 @@ func getMockRevive(t *testing.T) *revivelib.Revive {
9392
conf,
9493
true,
9594
2048,
95+
revivelib.NewExtraRule(&rule.IfReturnRule{}, lint.RuleConfig{}),
9696
revivelib.NewExtraRule(&mockRule{}, lint.RuleConfig{}),
9797
)
9898
if err != nil {

0 commit comments

Comments
 (0)