-
Notifications
You must be signed in to change notification settings - Fork 285
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
Conversation
internal/ifelse/rule.go
Outdated
|
||
func (v *visitor) checkRule(ifStmt *ast.IfStmt, chain Chain) { | ||
failMsg := v.rule.CheckIfElse(chain, v.args) | ||
if failMsg == "" { |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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)
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Thanks for the PR @mdelah thank you @alexandear and @ccoVeille for the reviews |
This PR implements an extension to the
early-return
proposed in #1133. Specifically, it suggest a rewrite of the formto
In cases where
if
statement is at the end of the surrounding function body,for
loop, orswitch
/select
branchelse
clause attachedif
block body consists of multiple statementsearly-return
are metCloses #1133