Rewrite lintElses check to handle multiple else ifs#181
Rewrite lintElses check to handle multiple else ifs#181zimmski wants to merge 1 commit intogolang:masterfrom
Conversation
There was a problem hiding this comment.
revert. map[T]bool is the most common way to represent a set, and the space savings for using map[T]struct{} are smaller than people tend to assume.
|
The code here is meant to completely ignore |
zimmski
force-pushed
the
fix-else-outdenting
branch
4 times, most recently
from
936d006 to
8fe77f2
Compare
|
@dsnet Any update on this? |
|
Any updates on this? |
|
@alandonovan can you take a look at this? |
alandonovan
left a comment
alandonovan
left a comment
There was a problem hiding this comment.
Code looks fine to me, but I'm not really an owner of this project.
|
|
||
| outdentElse := true | ||
|
|
||
| // The else-block should only be outdented if the if block ends with a return statement. |
There was a problem hiding this comment.
Is there are reason to detect 'return' but not other control statements such as 'continue' and 'break'?
There was a problem hiding this comment.
The original code handles only "returns". I guess it would be better to add "continue" and "break" in another PR unless you really want me to?
| if _, ok := ifStmt.Else.(*ast.BlockStmt); !ok { | ||
| // only care about elses without conditions | ||
|
|
||
| if _, ok := ignore[ifStmt]; ok { |
There was a problem hiding this comment.
It's quite normal to use a map[T]bool containing only true values to represent a set of T, and idiomatic to test for membership of such a set using just m[k]. Please restore the previous check.
|
@zimmski ping |
|
@googlebot cla? |
zimmski
force-pushed
the
fix-else-outdenting
branch
from
8fe77f2 to
653ab5c
Compare
zimmski
force-pushed
the
fix-else-outdenting
branch
from
653ab5c to
793a850
Compare
|
Funny enough, I did not receive any notifications for @alandonovan comments but got @andybons comments right away? So just in case my comments for the code are not going through: |
|
This PR (HEAD: 793a850) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/#/c/lint/+/96076 to see it. Tip: You can toggle comments from me using the |
|
This PR is being closed because golang.org/cl/96076 has been abandoned. Thank you for submitting this patch! As proposed[1], we are freezing and deprecating golint. There's no drop-in replacement to golint per se, but you should find that Staticcheck[2] works well in encouraging good Go code, much like golint did in the past, since it also includes style checks. There's always gofmt and "go vet" too, of course. If you would like to contribute further, I'd encourage you to engage Staticcheck's issue tracker[3] or look at vet's open issues[4], as they are both actively maintained. If you have an idea that doesn't fit into either of those tools, you could look at other Go linters[5], or write your own - these days it's fairly straightforward with go/analysis[6]. To help avoid confusion, I'm closing all CLs before we freeze the repository. If you have any feedback, you can leave a comment on the proposal thread where it was decided to deprecate golint - though note that the proposal has been accepted for nearly a year. Thanks! [1] golang/go#38968 |
7 participants
As suggested by @dsymonds this PR handles multiple else-ifs in lintElses. The suggestion for outdenting an else block is only shown if the if block has a return as last statement, no else-ifs and of course an else block.The PR also adds some test cases to make the behavior clearer.