Fix formatter comment handling around SQL expressions#884
Open
mk3008 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR fixes SQL formatter comment handling to eliminate duplicate comments in formatted output. Parser updates capture comments from LIMIT/OFFSET keywords and attach them to their values. Token printing logic deduplicates comments across nested components. Indentation rules adjust for SelectItem contexts. Comprehensive tests verify correct behavior across multiple SQL constructs. ChangesSQL Formatter Comment Handling Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/tests/transformers/CommentStyle.comprehensive.test.ts`:
- Around line 161-186: The test currently only checks presence of the comments
but should ensure they appear exactly once and directly adjacent to their
corresponding LIMIT/OFFSET parameters; update the assertions on
result.formattedSql (produced by formatter.format(query) where query =
SelectQueryParser.parse(sql).toSimpleQuery()) to (1) assert a single occurrence
of "page size" and "page offset" (e.g. count/indexOf checks) and (2) assert each
comment appears immediately before its parameter (e.g. assert the substring
"limit /* page size */ :limit" and "offset /* page offset */ :offset" or
equivalent regex matches) so duplication or misplacement will fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d8a697a-bd7f-4d4c-91ac-b2ee9e6f2335
📒 Files selected for processing (7)
.changeset/fix-comment-formatting.mdpackages/core/src/parsers/LimitClauseParser.tspackages/core/src/parsers/OffsetClauseParser.tspackages/core/src/parsers/SqlPrintTokenParser.tspackages/core/src/transformers/SqlPrinter.tspackages/core/tests/transformers/CommentStyle.comprehensive.test.tspackages/core/tests/transformers/SqlFormatter.comprehensive.test.ts
Comment on lines
+161
to
+186
| test('should preserve comments before LIMIT and OFFSET values', () => { | ||
| const formatter = new SqlFormatter({ | ||
| exportComment: true, | ||
| commentStyle: 'block', | ||
| commaBreak: 'before', | ||
| indentSize: 4, | ||
| indentChar: ' ', | ||
| keywordCase: 'lower', | ||
| newline: '\n' | ||
| }); | ||
| const sql = ` | ||
| select * | ||
| from t | ||
| order by id | ||
| limit /* page size */ | ||
| :limit | ||
| offset /* page offset */ | ||
| :offset | ||
| `; | ||
|
|
||
| const query = SelectQueryParser.parse(sql).toSimpleQuery(); | ||
| const result = formatter.format(query); | ||
|
|
||
| expect(result.formattedSql).toContain('page size'); | ||
| expect(result.formattedSql).toContain('page offset'); | ||
| }); |
There was a problem hiding this comment.
Strengthen LIMIT/OFFSET assertions to catch duplication and misplacement.
Line 184 and Line 185 only verify presence. This still passes if comments are duplicated or moved away from the LIMIT/OFFSET values, which weakens this regression.
Suggested assertion tightening
- expect(result.formattedSql).toContain('page size');
- expect(result.formattedSql).toContain('page offset');
+ expect(result.formattedSql.match(/page size/g)).toHaveLength(1);
+ expect(result.formattedSql.match(/page offset/g)).toHaveLength(1);
+ expect(result.formattedSql).toMatch(/limit\s+\/\*\s*page size\s*\*\/\s*:limit/i);
+ expect(result.formattedSql).toMatch(/offset\s+\/\*\s*page offset\s*\*\/\s*:offset/i);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('should preserve comments before LIMIT and OFFSET values', () => { | |
| const formatter = new SqlFormatter({ | |
| exportComment: true, | |
| commentStyle: 'block', | |
| commaBreak: 'before', | |
| indentSize: 4, | |
| indentChar: ' ', | |
| keywordCase: 'lower', | |
| newline: '\n' | |
| }); | |
| const sql = ` | |
| select * | |
| from t | |
| order by id | |
| limit /* page size */ | |
| :limit | |
| offset /* page offset */ | |
| :offset | |
| `; | |
| const query = SelectQueryParser.parse(sql).toSimpleQuery(); | |
| const result = formatter.format(query); | |
| expect(result.formattedSql).toContain('page size'); | |
| expect(result.formattedSql).toContain('page offset'); | |
| }); | |
| test('should preserve comments before LIMIT and OFFSET values', () => { | |
| const formatter = new SqlFormatter({ | |
| exportComment: true, | |
| commentStyle: 'block', | |
| commaBreak: 'before', | |
| indentSize: 4, | |
| indentChar: ' ', | |
| keywordCase: 'lower', | |
| newline: '\n' | |
| }); | |
| const sql = ` | |
| select * | |
| from t | |
| order by id | |
| limit /* page size */ | |
| :limit | |
| offset /* page offset */ | |
| :offset | |
| `; | |
| const query = SelectQueryParser.parse(sql).toSimpleQuery(); | |
| const result = formatter.format(query); | |
| expect(result.formattedSql.match(/page size/g)).toHaveLength(1); | |
| expect(result.formattedSql.match(/page offset/g)).toHaveLength(1); | |
| expect(result.formattedSql).toMatch(/limit\s+\/\*\s*page size\s*\*\/\s*:limit/i); | |
| expect(result.formattedSql).toMatch(/offset\s+\/\*\s*page offset\s*\*\/\s*:offset/i); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/tests/transformers/CommentStyle.comprehensive.test.ts` around
lines 161 - 186, The test currently only checks presence of the comments but
should ensure they appear exactly once and directly adjacent to their
corresponding LIMIT/OFFSET parameters; update the assertions on
result.formattedSql (produced by formatter.format(query) where query =
SelectQueryParser.parse(sql).toSimpleQuery()) to (1) assert a single occurrence
of "page size" and "page offset" (e.g. count/indexOf checks) and (2) assert each
comment appears immediately before its parameter (e.g. assert the substring
"limit /* page size */ :limit" and "offset /* page offset */ :offset" or
equivalent regex matches) so duplication or misplacement will fail the test.
mk3008
force-pushed
the
codex/fix-comment-formatting
branch
from
2288ee3 to
4c2d0f4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
pnpm vitest run packages/core/tests/transformers/CommentStyle.comprehensive.test.ts packages/core/tests/transformers/SqlFormatter.comprehensive.test.ts packages/core/tests/transformers/SqlFormatter.case-comment-regression.test.ts packages/core/tests/transformers/SqlFormatter.comment-placement.test.ts packages/core/tests/transformers/SqlFormatter.comment-exact-transformation.test.ts packages/core/tests/transformers/SqlFormatter.comment-statement-invariance.property.test.ts packages/core/tests/transformers/SqlFormatter.values-comment-indentation.test.ts packages/core/tests/transformers/SqlFormatter.merge.test.tspnpm --filter rawsql-ts testpnpm typecheckMerge Readiness
Tracking issue: Not required
Scoped checks run: Focused formatter tests,
pnpm --filter rawsql-ts test,pnpm typecheck, pre-commit workspace typecheck/test/build/lintWhy full baseline is not required: No exception requested; pre-commit ran the workspace gate successfully.
Self Review
Self-review workflow: self-review skill, two-cycle review after formatter-focused tests and the full pre-commit gate.
Self-review result: No blockers found; evidence covers the comment-loss, duplicate-comment, CASE indentation, and before/after comma layout regressions.
Concept-review workflow: No concept review required; this is a rawsql-ts formatter bug fix with no concept, package-boundary, CLI, or scaffold contract change.
Concept-review result: No concept or package-boundary violations found.
CLI Surface Migration
No-migration rationale: No CLI command, option, or public user-facing contract changed; formatter behavior is corrected.
Upgrade note: Not required.
Deprecation/removal plan or issue: Not required.
Docs/help/examples updated: Not required; changeset added for release notes.
Release/changeset wording:
.changeset/fix-comment-formatting.mdScaffold Contract Proof
No-proof rationale: No scaffold templates, generation contracts, or scaffolded output changed.
Non-edit assertion: Not required.
Fail-fast input-contract proof: Not required.
Generated-output viability proof: Not required.