docs+build: introduce ast-grep to encode some of the style guide elements not captured by the linter #10393
Conversation
This commit introduces ast-grep (sg) rules for automated style enforcement, encoding many of the patterns documented in docs/development_guidelines.md as machine-checkable rules. The rules/ directory contains two rule files: go-formatting.yml enforces: - struct-literal-asymmetric-close: Multi-line struct literals need the closing brace on its own line - func-call-asymmetric-wrap: Multi-line function calls need symmetric wrapping (excludes log/error calls which have special compact rules) - log-error-expanded-form: Hints when log/error calls use expanded form instead of the preferred compact form - switch-case-needs-spacing: Switch cases should have blank lines between - select-case-needs-spacing: Select cases should have blank lines between - no-inline-comments: Comments should be on their own line above code go-func-def.yml enforces: - func-def-dangling-param-comma: No dangling comma before close paren - func-def-dangling-return-paren: Return types shouldn't start with open paren on its own line The sgconfig.yml points ast-grep to the rules/ directory for scanning.
This commit adds two new Makefile targets for running ast-grep: - make ast-lint: Run ast-grep style checks against the codebase - make ast-grep-fix: Auto-fix safe style issues Both targets support an optional pkg parameter to focus on a specific directory (e.g., make ast-lint pkg=lnwallet). Generated files like *.pb.go are automatically excluded from scanning.
Summary of ChangesHello @Roasbeef, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces ast-grep to automate style guide enforcement, which is an excellent improvement for maintaining code quality and consistency. The new Makefile targets are well-implemented, and the ast-grep rules you've written seem to correctly capture many of the formatting guidelines from docs/development_guidelines.md. My review includes a few suggestions to adjust the severity of some rules to better align with the project's review standards.
| # Per guidelines, log and error formatting should minimize lines. | ||
| id: log-error-expanded-form | ||
| language: go | ||
| severity: hint |
There was a problem hiding this comment.
The severity for this rule is set to hint. Given the project's code review standards to flag issues of medium severity and higher, this might be too low to be actionable. Consider raising it to warning to ensure these style violations are addressed.1
severity: warningStyle Guide References
Footnotes
-
The style guide recommends minimizing lines for log and error messages, preferring a compact format over an expanded one. ↩
| # Detects consecutive case clauses without blank line spacing. | ||
| id: switch-case-needs-spacing | ||
| language: go | ||
| severity: hint |
There was a problem hiding this comment.
The severity for this rule is hint. To better enforce the style guide's preference for spacing between switch cases, please consider increasing the severity to warning. This will make the rule more effective in automated checks, aligning with the goal of flagging medium severity issues and higher.1
severity: warningStyle Guide References
Footnotes
-
The style guide recommends adding spacing between case stanzas in switch and select statements to improve readability. ↩
| # Rule: Select cases should be separated by blank lines. | ||
| id: select-case-needs-spacing | ||
| language: go | ||
| severity: hint |
There was a problem hiding this comment.
Similar to the switch-case-needs-spacing rule, the severity here is hint. It is recommended to change this to warning to ensure consistent enforcement of the style guide's spacing rules for select statements.1
severity: warningStyle Guide References
Footnotes
-
The style guide recommends adding spacing between case stanzas in switch and select statements to improve readability. ↩
| sg scan $(AST_GREP_EXCLUDE) $(AST_GREP_PATH) | ||
|
|
||
| #? ast-grep-fix: Auto-fix ast-grep style issues. Use pkg=<dir> to focus on a specific directory. | ||
| ast-grep-fix: |
There was a problem hiding this comment.
Tried make ast-grep-fix pkg=lnwallet but it didn't fix.
| AST_GREP_PATH := $(if $(pkg),$(pkg),.) | ||
|
|
||
| #? ast-lint: Run ast-grep style checks. Use pkg=<dir> to focus on a specific directory. | ||
| ast-lint: |
There was a problem hiding this comment.
Looks like it giving false positives, or should we update our guidelines to be more specific?
warning[func-def-dangling-return-paren]: Function return types should not start with open paren on its own line
┌─ lnwallet/channel_test.go:1625:1
│
1625 │ ╭ func TestHTLCSigNumber(t *testing.T) {
· │
1775 │ │ }
│ ╰─^
│
= Per docs/development_guidelines.md: Return type wrapping should continue
on the same line, not start a new line with open paren.
WRONG:
func bar(a, b, c) (
d, error,
) {
RIGHT:
func baz(a, b, c) (d,
error) {
2 participants
In this PR we introduce ast-grep (sg) for automated style enforcement, encoding many of the formatting patterns documented in
docs/development_guidelines.mdas machine-checkable rules. The rules cover multi-line function call wrapping, struct literal formatting, switch/select case spacing, inline comments, and function definition wrapping. Log and error calls are correctly excluded from the symmetric wrapping rules since they follow different compact formatting guidelines.Two new Makefile targets are added: make ast-lint to check for style issues and make ast-grep-fix to auto-fix safe violations. Both support a pkg parameter to focus on specific directories (e.g., make ast-lint pkg=wallet). Generated files like *.pb.go and sqlc output are automatically excluded from scanning.
Here's an example of a violation it finds based on our guidelines: