Skip to content

Apply various formatting fixes #80

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

Merged
nene merged 5 commits into from
Sep 7, 2026
Merged

Apply various formatting fixes #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f333c58
Match WHEN/THEN/ELSE clause formatting with procedural style
joelmukuthu Aug 10, 2026
d830774
Break long COMMENT ON clauses into separate lines
joelmukuthu Aug 10, 2026
b1a2322
Break long CREATE INDEX clauses into separate lines
joelmukuthu Aug 10, 2026
c9069a4
Break long binary expressions into separate lines
joelmukuthu Aug 10, 2026
868b3f9
Avoid line-break between empty parenthesis
joelmukuthu Aug 10, 2026
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
9 changes: 6 additions & 3 deletions src/syntax/comment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { AllCommentNodes } from "sql-parser-cst";
import { group, indent, line } from "../print_utils";
import { group, line } from "../print_utils";
import { CstToDocMap } from "../CstToDocMap";

export const commentMap: CstToDocMap<AllCommentNodes> = {
comment_stmt: (print) =>
group([
print.spaced(["commentKw", "onKw", "target", "isKw"]),
indent([line, print("message")]),
print.spaced(["commentKw", "onKw"]),
line,
print("target"),
line,
print.spaced(["isKw", "message"]),
]),

comment_target_aggregate: (print) =>
Expand Down
72 changes: 53 additions & 19 deletions src/syntax/expr.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
Keyword,
Node,
Parameter,
ParenExpr,
Variable,
} from "sql-parser-cst";
import { CstToDocMap } from "../CstToDocMap";
Expand Down Expand Up @@ -83,6 +84,9 @@ export const exprMap: CstToDocMap<AllExprNodes> = {
) {
return print("expr");
}
if (isEmptyParenExpr(node)) {
return ["(", print("expr"), ")"];
}
const lineStyle =
isCreateTableStmt(parent) && print.dynamicLine() === hardline
? hardline
Expand All @@ -101,7 +105,10 @@ export const exprMap: CstToDocMap<AllExprNodes> = {
// Some operators are better formatted without spaces around them
return print(["left", "operator", "right"]);
}
return print.spaced(["left", "operator", "right"]);
return group([
print("left"),
group([" ", print.spaced("operator"), indent([line, print("right")])]),
]);
},
prefix_op_expr: (print, node) =>
(isString(node.operator) ? print : print.spaced)(["operator", "expr"]),
Expand All @@ -114,24 +121,28 @@ export const exprMap: CstToDocMap<AllExprNodes> = {
hardline,
print("endKw"),
],
case_when: (print, node) => {
if (isProgram(node.result)) {
return [
print.spaced(["whenKw", "condition", "thenKw"]),
indent([hardline, stripTrailingHardline(print("result"))]),
];
}
return print.spaced(["whenKw", "condition", "thenKw", "result"]);
},
case_else: (print, node) => {
if (isProgram(node.result)) {
return [
print("elseKw"),
indent([hardline, stripTrailingHardline(print("result"))]),
];
}
return print.spaced(["elseKw", "result"]);
},
case_when: (print, node) => [
group([
group([print("whenKw"), indent([line, print("condition")])]),
line,
print("thenKw"),
]),
indent([
hardline,
isProgram(node.result)
? stripTrailingHardline(print("result"))
: print("result"),
]),
],
case_else: (print, node) => [
print("elseKw"),
indent([
hardline,
isProgram(node.result)
? stripTrailingHardline(print("result"))
: print("result"),
]),
],
member_expr: (print, node) =>
isArraySubscript(node.property)
? print(["object", "property"])
Expand Down Expand Up @@ -317,3 +328,26 @@ const isFunctionContext = (
const isBooleanOp = ({ name }: Keyword) => name === "AND" || name === "OR";

const isCompactOp = (op: string) => op === "->" || op === "->>";

const hasComments = (node: Node): boolean =>
(node.leading?.length ?? 0) > 0 || (node.trailing?.length ?? 0) > 0;

const isEmptyParenExpr = (node: ParenExpr): boolean => {
if (hasComments(node) || hasComments(node.expr)) {
return false;
}
if (isFuncArgs(node.expr)) {
return (
node.expr.args.items.length === 0 &&
!node.expr.distinctKw &&
!node.expr.nullHandlingKw &&
!node.expr.orderBy &&
!node.expr.limit &&
!node.expr.having
);
}
if (isListExpr(node.expr)) {
return node.expr.items.length === 0;
}
return false;
};
36 changes: 21 additions & 15 deletions src/syntax/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,30 @@
import { AllIndexNodes } from "sql-parser-cst";
import { group, join } from "../print_utils";
import { group, join, line } from "../print_utils";
import { CstToDocMap } from "../CstToDocMap";

export const indexMap: CstToDocMap<AllIndexNodes> = {
create_index_stmt: (print) =>
create_index_stmt: (print, node) =>
group(
join(print.dynamicLine(), [
print.spaced([
"createKw",
"orReplaceKw",
"indexTypeKw",
"indexKw",
"concurrentlyKw",
"ifNotExistsKw",
"name",
"onKw",
"table",
"using",
"columns",
]),
group(
join(line, [
print.spaced([
"createKw",
"orReplaceKw",
"indexTypeKw",
"indexKw",
"concurrentlyKw",
"ifNotExistsKw",
"name",
]),
print.spaced([
"onKw",
"table",
...(node.using ? [] : (["columns"] as const)),
]),
...(node.using ? [print.spaced(["using", "columns"])] : []),
]),
),
...print("clauses"),
]),
),
Expand Down
3 changes: 2 additions & 1 deletion test/ddl/create_table.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,8 @@ describe("create table", () => {
OPTIONS (
expiration_timestamp = TIMESTAMP "2025-01-01 00:00:00 UTC",
partition_expiration_days = 1,
description = "a table that expires in 2025, with each partition living for 24 hours",
description =
"a table that expires in 2025, with each partition living for 24 hours",
labels = [("org_unit", "development")]
)
`);
Expand Down
46 changes: 38 additions & 8 deletions test/ddl/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,49 @@ describe("index", () => {
`);
});

it(`breaks long CREATE INDEX across CREATE, ON, and USING`, async () => {
await testPostgresql(
dedent`
CREATE INDEX my_index
ON my_table
USING btree (col)
`,
{ printWidth: 50 },
);
});

it(`breaks long CREATE UNIQUE INDEX across CREATE, ON, and USING`, async () => {
await testPostgresql(
dedent`
CREATE UNIQUE INDEX my_index
ON my_table
USING btree (
col_one,
col_two
)
`,
{ printWidth: 30 },
);
});

it(`formats long columns list on multiple lines`, async () => {
await test(dedent`
CREATE UNIQUE INDEX IF NOT EXISTS my_index ON my_table (
column_name_one,
column_name_two,
column_name_three
)
`);
await test(
dedent`
CREATE UNIQUE INDEX IF NOT EXISTS my_index
ON my_table (
col_one,
col_two,
col_three
)
`,
{ printWidth: 40 },
);
});

it(`formats column list with various index parameters`, async () => {
await testPostgresql(dedent`
CREATE INDEX my_index ON my_table (
CREATE INDEX my_index
ON my_table (
column_name_one COLLATE "C" ASC NULLS FIRST,
column_name_two DESC NULLS LAST,
(col3 + col4) my_opclass (foo = 'bar', baz = 'qux') ASC
Expand Down
105 changes: 97 additions & 8 deletions test/expr/expr.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,27 @@ describe("expr", () => {
`);
});

it(`keeps short binary expressions on one line`, async () => {
await test(dedent`
SELECT *
FROM foo
WHERE bar = short_func()
`);
});

it(`breaks long binary expressions into multiple lines with indentation`, async () => {
await test(
dedent`
SELECT *
FROM foo
WHERE
bar =
my_func()
`,
{ printWidth: 15 },
);
});

it(`formats IN expressions`, async () => {
await test(`SELECT col1 IN (1, 2, 3), col2 NOT IN (4, 5, 6)`);
});
Expand Down Expand Up @@ -144,8 +165,10 @@ describe("expr", () => {
await test(dedent`
SELECT
CASE x
WHEN 1 THEN 'A'
ELSE 'B'
WHEN 1 THEN
'A'
ELSE
'B'
END
Comment on lines 165 to 172

@nene nene Sep 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think I gave you the wrong idea when I said we should format the case expression the same as the case statement in procedural SQL. What I meant was, that we should format them similarly when they are so long that they don't fit on a single line. That is, I'd still expect to format the above as:

CASE x
  WHEN 1 THEN 'A'
  ELSE 'B'
END

Only when the expression doesn't fit on a single line should be break the WHEN..THEN or ELSE block to multiple lines like so:

CASE x
  WHEN 1 THEN
    'Something long in here'
  ELSE
    'Another long thing in here'
END

or when the condition part is long:

CASE x
  WHEN some_long_expression_in_here THEN
    'A'
  ELSE
    'B'
END

or extra long:

CASE x
  WHEN
    some_extra_long_expression_in_here
  THEN
    'A'
  ELSE
    'B'
END

@joelmukuthu joelmukuthu Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What I meant was, that we should format them similarly when they are so long that they don't fit on a single line.

This was what I understood at first, but I changed the implementation because of the desired ELSE formatting. With the current code setup, the ELSE clause is formatted separately/independently, so a "short" ELSE gets formatted as follows (regardless of THENs formatting):

CASE x
  WHEN some_long_expression_in_here THEN
    'A'
  ELSE 'B'
END

Alternatively, the current implementation could be changed to couple WHEN/THEN/ELSE formatting, so we can know to break the ELSE statement if the WHEN/THEN statements also break.

Or alternatively, always break the statements, which also happens to look like if..else statements (with braces). So when I got to this point I thought this what you meant all along :)

Anyway to summarise, I think the options are:

  1. Format WHEN/THEN and ELSE independently and acknowledge that sometimes the ELSE will be formatted differently from the WHEN/THEN statements
  2. Update the formatting code to couple WHEN/THEN/ELSE formatting. I didn't explore this too much but I felt like it would be a non-trivial overhaul -- I could be wrong
  3. Always break THEN and ELSE clauses to the next line
  4. Maybe something else?

@nene nene Sep 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yeah, I'm leaning towards option 2. But I also don't really know how to easily achieve that. Although I'm not 100% sure we want that. The bad thing with this variant is that making one WHEN-block longer would cause a large reindent for all of them. That might or might not be what one desires. Like, if one has a long CASE expression with lots of small WHEN-THEN blocks, and then one that's a bit longer, it could be annoying to have everything split to multiple lines because of that one block. On the other hand, it can be annoying to have a mix of one-line and two-line WHEN-THEN blocks.

But option 1 is definitely the next best thing. And maybe it's even the better variant out of the two. Let's go with that and not try to fight too much with the general indentation approach of Prettier.

`);
});
Expand All @@ -154,9 +177,12 @@ describe("expr", () => {
await test(dedent`
SELECT
CASE status
WHEN 1 THEN 'good'
WHEN 2 THEN 'bad'
ELSE 'unknown'
WHEN 1 THEN
'good'
WHEN 2 THEN
'bad'
ELSE
'unknown'
END
`);
});
Expand All @@ -165,12 +191,75 @@ describe("expr", () => {
await test(dedent`
SELECT
CASE
WHEN status = 1 THEN 'good'
WHEN status = 2 THEN 'bad'
ELSE 'unknown'
WHEN status = 1 THEN
'good'
WHEN status = 2 THEN
'bad'
ELSE
'unknown'
END
`);
});

it(`breaks long WHEN/THEN into separate lines`, async () => {
await test(
dedent`
SELECT
CASE
WHEN column_name = 1 THEN
result_name
END
`,
{ printWidth: 40 },
);
});

it(`breaks multiple long WHEN/THEN clauses without blank lines between them`, async () => {
await test(
dedent`
SELECT
CASE
WHEN column_name = 1 THEN
result_name
WHEN column_name = 2 THEN
other_result
ELSE
foo
END
`,
{ printWidth: 40 },
);
});
Comment on lines +183 to +211

@joelmukuthu joelmukuthu Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think these tests have become obsolete now that we always break. Best to remove


it(`indents multi-condition WHEN clauses and keeps ORs parenthesized`, async () => {
await test(
dedent`
SELECT
CASE
WHEN
column_name = 1
AND (other_name = 2 OR other_name = 3)
THEN
result_name
END
`,
{ printWidth: 50 },
);
});

it(`indents multi-expression THEN clauses and keeps ORs parenthesized`, async () => {
await test(
dedent`
SELECT
CASE
WHEN column_name = 1 THEN
result_name = 1
AND (other_name = 2 OR other_name = 3)
END
`,
{ printWidth: 45 },
);
});
});

it(`formats quantifier expressions`, async () => {
Expand Down
Loading
Loading