Drop if-return from default ruleset #843
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
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
10 tasks
raghavendra-talur
added a commit
to raghavendra-talur/ramen
that referenced
this pull request
Jan 9, 2025
if-return has been removed from the default list of revive for a valid reason. See the argument in mgechev/revive#843. Comparing two snippets 1. ``` if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteSnapshots(namespace); err != nil { return err } return nil ``` 2. ``` if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil { return err } return v.volSyncHandler.DeleteSnapshots(namespace) ``` 1 is a lot easier to read compared to 2 but if-return throws an error for it. Signed-off-by: Raghavendra Talur <[email protected]>
raghavendra-talur
added a commit
to raghavendra-talur/ramen
that referenced
this pull request
Jan 9, 2025
if-return has been removed from the default list of revive for a valid reason. See the argument in mgechev/revive#843. Comparing two snippets 1. ``` if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteSnapshots(namespace); err != nil { return err } return nil ``` 2. ``` if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil { return err } return v.volSyncHandler.DeleteSnapshots(namespace) ``` 1 is a lot easier to read compared to 2 but if-return throws an error for it. Signed-off-by: Raghavendra Talur <[email protected]>
raghavendra-talur
added a commit
to raghavendra-talur/ramen
that referenced
this pull request
Jan 10, 2025
if-return has been removed from the default list of revive for a valid reason. See the argument in mgechev/revive#843. Comparing two snippets 1. ``` if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteSnapshots(namespace); err != nil { return err } return nil ``` 2. ``` if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil { return err } return v.volSyncHandler.DeleteSnapshots(namespace) ``` 1 is a lot easier to read compared to 2 but if-return throws an error for it. Signed-off-by: Raghavendra Talur <[email protected]>
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.
The
if-returnrule 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:
The
if-returnrule considers this a violation of style, and instead suggests the following: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