-
Notifications
You must be signed in to change notification settings - Fork 46
Add support for newline-after-block
#221
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
Conversation
This adds a new check type to enforce newline after any block (closing `}`). In addition, it also makes the check for empty line while using `case-max-lines` a bit more strict by now _not_ counting a trailing comment as an empty line. While doing this, there's a new column based logic to figure out where the newline should go in the end of a case. If it's aligned with the last statement of the case body the line is inserted after. If it's aligned with then next case arm, the line is inserted before the comment. While working on this we're also moving to a line based check that uses `LineStart` to make replacements and with that simplifying some comment parsing since we only need to know the pos where the line starts. This also allowed to add support to fix removal of whitespaces both before _and_ after leading comments in blocks.
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.
Pull request overview
This PR adds support for a new newline-after-block check that enforces a blank line after any block statement (closing }). The implementation includes refined handling of trailing comments in case clauses and switches to line-based logic using LineStart for more reliable fix positioning. The changes also improve comment parsing and support whitespace removal both before and after leading comments in blocks.
Changes:
- Added new
CheckNewlineAfterBlockcheck type that requires blank lines after block statements - Refactored
checkCaseTrailingNewlineto use column-based logic for comment classification (trailing vs leading) - Extracted
isErrNotNilCheckhelper function to detect error checking patterns and reuse across checks - Updated line-based whitespace handling using
LineStartfor precise fix positioning
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| wsl.go | Core implementation of newline-after-block check, refactored error checking logic, improved comment handling |
| config.go | Added CheckNewlineAfterBlock constant and string mapping |
| config_test.go | Updated max check number from 23 to 24 |
| analyzer_test.go | Added test configurations for all_enabled and newline_after_block scenarios |
| README.md | Documented the new check as disabled by default |
| CHECKS.md | Added comprehensive documentation for newline-after-block and case-max-lines |
| testdata/ | Comprehensive test coverage for the new check across various scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
We don't need to check line number to find relevant comments, if we instead use the `ast.Pos` as long as possible we save a ton of conversions and gets some speed improvement.
Removes extra check for statement type before calling function. Also adds a more detailed comment to why we skip complex groups.
|
Some new numbers after the recent change, still on
I have no clue how all checks can be faster than default checks (which disables some) on the However there's clearly some issue with how I handle trailing comments in case blocks which wasn't handled at all before. But it's ~3x slower now and it's also clear that where time is spent. It would be nice if enabling EDIT Oh, I was recreating a file comment map on every run. If I cache it we're back to normal times:
And if I don't create a comment map at all and iterate over
So I guess we can consider this no slowdown. EDIT 2: Found some speed improvement areas that might help depending on what checks you have enabled so created tickets for that to do as follow-ups. So all that's left now is to decide on the trailing comment handling in case blocks. |
ae585e9 to
63c7e0d
Compare
a2aeda0 to
a7bc7b7
Compare
Similar to how we do support trailing comments in a block, but no newline after the comment, we want to support left-aligned comments in case bodies but keep them flush against the next case.
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.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9c1660d to
edab8b3
Compare
This adds a new check type to enforce newline after any block (closing
}). In addition, it also makes the check for empty line while usingcase-max-linesa bit more strict by now not counting a trailing comment as an empty line.While doing this, there's a new column based logic to figure out where the newline should go in the end of a case. If it's aligned with the last statement of the case body the line is inserted after. If it's aligned with then next case arm, the line is inserted before the comment.
While working on this we're also moving to a line based check that uses
LineStartto make replacements and with that simplifying some comment parsing since we only need to know the pos where the line starts. This also allowed to add support to fix removal of whitespaces both before and after leading comments in blocks.This change does:
Rather simplified a lot of comment and line parsing. Some added complexity for case trailing comments.
Confirmed by adding tests enabling all checks.
The change seems to add 25% slowdown when enabled.
Average time in seconds over 10 runs example with cache on
prometheusrepo:Closes #204