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

feat: optional extension to early-return rule (#1133) #1138

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

mdelah
Copy link
Contributor

@mdelah mdelah commented Nov 19, 2024

This PR implements an extension to the early-return proposed in #1133. Specifically, it suggest a rewrite of the form

if x {
  y
}

to

if !x {
   return|break|continue
}
y

In cases where

  • the if statement is at the end of the surrounding function body, for loop, or switch / select branch
  • it has no else clause attached
  • the if block body consists of multiple statements
  • the other common prerequisites for early-return are met
  • the new behaviour has been opted into via a configuration argument

Closes #1133


func (v *visitor) checkRule(ifStmt *ast.IfStmt, chain Chain) {
failMsg := v.rule.CheckIfElse(chain, v.args)
if failMsg == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I'm not comfortable with "" as indicator of no failure. CheckIfElse could return a failure (or an error?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm a little unclear on what you're proposing. Per doc string

// CheckIfElse evaluates the rule against an ifelse.Chain and returns a failure message if applicable.

The term "failure" may be a bit distracting (it could better be termed "suggestion"), but there is no actual possibility of failure/error here, either there is a message, or there isn't.

Would you prefer (string, bool)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

a bool return for Check-something function sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I think my reservation with it is it forces us to add bools at a lot of return points within the implementation that end up being equivalent to testing the string against empty anyway. But see what you think. Possibly I've misunderstood what you were getting at.

I also changed the ifelse.Rule interface to a func since it was a bit unnecessary with a single method, and lets the implementations be unexported.

internal/ifelse/branch.go Outdated Show resolved Hide resolved
internal/ifelse/branch.go Outdated Show resolved Hide resolved
internal/ifelse/branch.go Show resolved Hide resolved
internal/ifelse/branch_kind.go Show resolved Hide resolved
internal/ifelse/rule.go Show resolved Hide resolved
internal/ifelse/rule.go Outdated Show resolved Hide resolved
@chavacava chavacava merged commit 7e1d35d into mgechev:master Nov 28, 2024
5 checks passed
@chavacava
Copy link
Collaborator

Thanks for the PR @mdelah

thank you @alexandear and @ccoVeille for the reviews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

early-return extension for end of surrounding block
4 participants