fix: keep a block comment leading a later statement on its own line - #961
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe formatter now tracks whether each statement follows another statement, passes that state into ChangesStatement layout formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
Thanks for the PR. Sorry for the lack of attention. I'm currently pretty busy. Maybe I'll get to reviewing this on Sunday or next week. |
nene
left a comment
There was a problem hiding this comment.
We don't really need to perform statement-counting to achieve this fix.
And we're lacking tests.
See the comment that outlines a simpler way to fix this problem.
| return item === WS.NEWLINE || item === WS.MANDATORY_NEWLINE; | ||
| } | ||
| return false; | ||
| return this.startsOnNewLine; |
There was a problem hiding this comment.
Looks like the same fix can be achieved by just replacing this with:
return true;But we're lacking on tests that would demonstrate a need for something more complex than that.
Layout case
This is definitely OK fix inside the Layout class, which gets constructed to format every separate SQL statement. And each statement will start from a new line, including the first one.
We might want to add a test that checks what happens when formatting a block-comment before a first SQL statement, like:
it('keeps first block comment on its own line', () => {
const sql = '/* c1 */ SELECT 1;';
const result = dedent`
/* c1 */
SELECT
1;
`;
expect(format(sql)).toBe(result);
});InlineLayout case
It's not so OK with the InlineLayout class, because that one is used to format expressions inside parenthesis, which never form a new line. But currently our tests don't cover this scenario well. We're missing a test like this:
it('preserves inline block-comment', () => {
const sql = 'SELECT (/* c */ 1 + 2);';
const result = dedent`
SELECT
(/* c */ 1 + 2);
`;
expect(format(sql)).toBe(result);
});To make that test pass, we should just override the isAtStartOfLine()method in InlineLayout with one that always returns false.
A block comment before a statement that follows another is formatted inline on the first pass but moved onto its own line on the second, so formatting isn't idempotent:
Same shape as #952, one level up. That fix made a block comment standalone when
layout.isAtStartOfLine(), butformatAstgives each statement its ownLayout, so for a leading comment the layout is still empty andisAtStartOfLine()falls through toreturn false. Statements are joined with newlines, so every statement after the first does start on a line — the second pass sees that newline as the comment'sprecedingWhitespaceand flips the decision.Layoutnow takes whether its statement is preceded by others, and an empty layout reportsisAtStartOfLine()accordingly. The first statement is unaffected, so a leading comment on the whole query still formats inline as before.Found by fuzzing
format(format(x)) === format(x)with comments injected at every token boundary: 14,427 variants over 14 dialects, 10 failures before, 0 after. It reproduces in 11 of 21 dialects and for any keyword-only statement —COMMIT,ROLLBACK,BEGIN,END,GO,USE,VACUUM,ANALYZE,SAVEPOINT. Statements with clauses are stable because the following keyword forces the newline anyway.5,861 tests pass; tsc, eslint and prettier clean.