Skip to content

fix: keep a block comment leading a later statement on its own line #961

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.

Sign up for GitHub

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

Open
MildlyMeticulous wants to merge 1 commit into
base: master
Choose a base branch
from
+23 −8
Open

fix: keep a block comment leading a later statement on its own line #961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/formatter/Formatter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,16 @@ export default class Formatter {

private formatAst(statements: StatementNode[]): string {
return statements
.map(stat => this.formatStatement(stat))
.map((stat, i) => this.formatStatement(stat, i > 0))
.join('\n'.repeat(this.cfg.linesBetweenQueries + 1));
}

private formatStatement(statement: StatementNode): string {
private formatStatement(statement: StatementNode, startsOnNewLine: boolean): string {
const layout = new ExpressionFormatter({
cfg: this.cfg,
dialectCfg: this.dialect.formatOptions,
params: this.params,
layout: new Layout(new Indentation(indentString(this.cfg))),
layout: new Layout(new Indentation(indentString(this.cfg)), startsOnNewLine),
}).format(statement.children);

if (!statement.hasSemicolon) {
Expand Down
12 changes: 7 additions & 5 deletions src/formatter/Layout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export type LayoutItem = WS.SPACE | WS.SINGLE_INDENT | WS.NEWLINE | WS.MANDATORY
export default class Layout {
private items: LayoutItem[] = [];

constructor(public indentation: Indentation) {}
constructor(public indentation: Indentation, private startsOnNewLine: boolean = false) {}

/**
* Appends token strings and whitespace modifications to SQL string.
Expand Down Expand Up @@ -123,9 +123,11 @@ export default class Layout {
}

/**
* True when some content has already been added and nothing but indentation
* has been emitted since the last newline, meaning the next token would be
* placed at the start of a fresh (non-first) line.
* True when nothing but indentation has been emitted since the last newline,
* meaning the next token would be placed at the start of a fresh line.
*
* An empty layout counts when the statement itself is preceded by others,
* since the formatter joins statements with newlines.
*/
public isAtStartOfLine(): boolean {
for (let i = this.items.length - 1; i >= 0; i--) {
Expand All @@ -135,7 +137,7 @@ export default class Layout {
}
return item === WS.NEWLINE || item === WS.MANDATORY_NEWLINE;
}
return false;
return this.startsOnNewLine;

@nene nene Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

private itemToString(item: LayoutItem): string {
Expand Down
13 changes: 13 additions & 0 deletions test/features/comments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,19 @@ export default function supportsComments(format: FormatFn, opts: CommentsConfig
expect(format(result)).toBe(result);
});

it('keeps a block comment leading a later statement on its own line', () => {
const sql = 'SELECT 1; /* c */ COMMIT;';
const result = dedent`
SELECT
1;

/* c */
COMMIT;
`;
expect(format(sql)).toBe(result);
expect(format(result)).toBe(result);
});

it('keeps block comments in various CREATE TABLE statement positions idempotent', () => {
const sql =
'CREATE TABLE /* c */ tbl (/* c */ id INT, /* c */ first_name TEXT, last_name TEXT)';
Expand Down
Loading