diff --git a/apps/desktop/src/lib/__tests__/sql/sqlFormatter.spec.ts b/apps/desktop/src/lib/__tests__/sql/sqlFormatter.spec.ts index a3703f8f9f..c976e545cf 100644 --- a/apps/desktop/src/lib/__tests__/sql/sqlFormatter.spec.ts +++ b/apps/desktop/src/lib/__tests__/sql/sqlFormatter.spec.ts @@ -159,13 +159,13 @@ describe("sqlFormatter", () => { }); it("keeps a view's trailing line comment from swallowing the next column (issue #10278)", async () => { - // A `--` comment consumes the rest of its line, so the column that follows - // it in the view text has to start a new line; appending it to the comment - // would silently drop it from the DDL shown to the user. + // A `--` comment consumes the rest of its line. Keeping it on the previous + // field's line is safe only when the following column starts a new line, + // otherwise the column would silently disappear from the DDL shown to the user. const viewDdl = `CREATE VIEW "TEMP_TEST_VIEW" AS SELECT trunc(sysdate) AS dates, -- 测试\n(SELECT sysdate FROM dual t) AS nows\nFROM dual;`; const formatted = await formatSqlForDisplay(viewDdl, sqlFormatDialectForDbType("oracle")); - expect(formatted).toBe('CREATE VIEW "TEMP_TEST_VIEW" AS\nSELECT trunc(sysdate) AS dates,\n -- 测试\n (SELECT sysdate FROM dual t) AS nows\nFROM dual;'); + expect(formatted).toBe('CREATE VIEW "TEMP_TEST_VIEW" AS\nSELECT trunc(sysdate) AS dates, -- 测试\n (SELECT sysdate FROM dual t) AS nows\nFROM dual;'); }); it("keeps a short OceanBase Oracle view projection readable (issue #7540)", async () => { @@ -505,6 +505,22 @@ LIMIT 100;`); await expect(formatSqlForEditing("SELECT 1 .\n", "dameng")).resolves.toBe("SELECT 1 .\n"); }); + it("keeps trailing SELECT comments on the field line (#12615)", async () => { + const source = `select a, -- first field\n b, -- second field\n c -- third field\nfrom t;`; + const formatted = await formatSqlForEditing(source, "postgres"); + + expect(formatted).toContain("SELECT a, -- first field\n"); + expect(formatted).toContain(" b, -- second field\n"); + expect(formatted).toContain(" c -- third field\nFROM t;"); + }); + + it("keeps source-standalone SELECT comments on their own line", async () => { + const source = `select a,\n-- heading for b\n b\nfrom t;`; + const formatted = await formatSqlForEditing(source, "postgres"); + + expect(formatted).toContain("SELECT a,\n -- heading for b\n b\nFROM t;"); + }); + it("preserves the newline before a trailing dot after a line comment", async () => { await expect(formatSqlForEditing("DELETE FROM accounts -- comment\n .", "dameng")).resolves.toBe("DELETE FROM accounts -- comment\n ."); }); diff --git a/apps/desktop/src/lib/__tests__/sql/sqlLayout.spec.ts b/apps/desktop/src/lib/__tests__/sql/sqlLayout.spec.ts index 8e67d4cda0..8757b56a2d 100644 --- a/apps/desktop/src/lib/__tests__/sql/sqlLayout.spec.ts +++ b/apps/desktop/src/lib/__tests__/sql/sqlLayout.spec.ts @@ -285,10 +285,10 @@ describe("sql layout", () => { expect(await format("SELECT a FROM t UNION ALL SELECT b FROM u;")).toBe(lines("SELECT a", "FROM t", "UNION ALL", "SELECT b", "FROM u;")); }); - it("keeps comments and does not fold them onto a line", async () => { + it("keeps a trailing comment on its line without swallowing the next item", async () => { const sql = lines("-- heading", "SELECT a, -- why", " b", "FROM t; /* trailing */"); - expect(await format(sql)).toBe(lines("-- heading", "SELECT a,", " -- why", " b", "FROM t;", "", "/* trailing */")); + expect(await format(sql)).toBe(lines("-- heading", "SELECT a, -- why", " b", "FROM t;", "", "/* trailing */")); }); it("keeps PostgreSQL casts tight", async () => { diff --git a/apps/desktop/src/lib/sql/layout/printer.ts b/apps/desktop/src/lib/sql/layout/printer.ts index a817e94cc6..c91cc79beb 100644 --- a/apps/desktop/src/lib/sql/layout/printer.ts +++ b/apps/desktop/src/lib/sql/layout/printer.ts @@ -78,10 +78,35 @@ function printClause(writer: Writer, clause: ClauseNode, baseColumn: number, ctx } items.forEach((item, index) => { + // A line comment written after the previous field's comma is a trailing + // annotation for that field, not a separate heading. sql-formatter keeps + // its original preceding whitespace; only comments that did not start a + // new source line are moved back onto the preceding output line. Comments + // that were on their own line keep their own line. + const leadingComments = index > 0 && ctx.options.commaPosition !== "before" ? leadingTrailingComments(item) : 0; + let itemNodes = item; + let leadingText: string | null = null; + if (leadingComments > 0) { + // `renderInline` intentionally drops comments; a trailing comment is + // copied verbatim, and only when it fits the remaining line width. + const commentText = item + .slice(0, leadingComments) + .map((node) => ("text" in node ? node.text : "")) + .join(" "); + if (commentText && writer.column + 1 + commentText.length <= ctx.options.lineWidth) { + leadingText = commentText; + itemNodes = item.slice(leadingComments); + } + } if (index === 0) { writer.write(keyword); if (sourceOnOwnLine) writer.newline(baseColumn + ctx.options.indentWidth); else writer.space(); + } else if (leadingText) { + writer.space(); + writer.write(leadingText); + if (itemNodes.length === 0) return; + writer.newline(itemColumn); } else { if (ctx.options.commaPosition === "before") { const leadingIndent = sourceOnOwnLine ? baseColumn + ctx.options.indentWidth : Math.max(baseColumn, itemColumn - 2); @@ -91,22 +116,34 @@ function printClause(writer: Writer, clause: ClauseNode, baseColumn: number, ctx writer.newline(itemColumn); } } - if (alignedAliases) { + if (alignedAliases && !leadingText) { const aligned = alignedAliases[index]; if (aligned) { writer.write(aligned.expression); writer.write(" ".repeat(aligned.aliasColumn - aligned.expression.length)); writer.write(`${aligned.asKeyword} ${aligned.alias}`); } else { - printElement(writer, item, baseColumn, childCtx); + printElement(writer, itemNodes, baseColumn, childCtx); } } else { - printElement(writer, item, baseColumn, childCtx); + printElement(writer, itemNodes, baseColumn, childCtx); } if (ctx.options.commaPosition !== "before" && index < items.length - 1) writer.write(","); }); } +/** Count leading comments that were written on the previous source line. */ +function leadingTrailingComments(item: AstNode[]): number { + let count = 0; + while (count < item.length) { + const node = item[count]; + if (!node || (node.type !== "line_comment" && node.type !== "block_comment")) break; + if (/[\r\n]/.test(node.precedingWhitespace ?? "")) break; + count += 1; + } + return count; +} + interface AlignedProjectionItem { expression: string; alias: string; diff --git a/packages/app-tests/sqlFormatter.test.ts b/packages/app-tests/sqlFormatter.test.ts index 6940f856e4..b8f7775ab1 100644 --- a/packages/app-tests/sqlFormatter.test.ts +++ b/packages/app-tests/sqlFormatter.test.ts @@ -76,14 +76,13 @@ test("rejects very large SQL before loading formatter work", async () => { await assert.rejects(() => formatSqlText("x".repeat(MAX_SQL_FORMAT_CHARS + 1), "generic"), /too large/i); }); -test("keeps a trailing line comment from swallowing the column after it", async () => { - // Oracle stores a view's text as written, so a comment at the end of a column - // line is followed by the next column on its own line. A `--` comment consumes - // the rest of its line, so the next column has to start a new line instead of - // being appended to that comment. +test("keeps a trailing line comment on its field line without swallowing the next column", async () => { + // A `--` comment consumes the rest of its source line. When the comment was + // written after the field's comma, keep it there and put the next column on + // a new line so it is still valid SQL (#12615). const formatted = await formatSqlText('CREATE VIEW "TEMP_TEST_VIEW" AS SELECT trunc(sysdate) AS dates, -- 测试\n(SELECT sysdate FROM dual t) AS nows\nFROM dual;', "oracle", { keywordCase: "upper" }); - assert.equal(formatted, 'CREATE VIEW "TEMP_TEST_VIEW" AS\nSELECT trunc(sysdate) AS dates,\n -- 测试\n (SELECT sysdate FROM dual t) AS nows\nFROM dual;'); + assert.equal(formatted, 'CREATE VIEW "TEMP_TEST_VIEW" AS\nSELECT trunc(sysdate) AS dates, -- 测试\n (SELECT sysdate FROM dual t) AS nows\nFROM dual;'); }); test("keeps a line comment from swallowing the separator after it", async () => { @@ -98,10 +97,10 @@ test("keeps a line comment from swallowing the statement terminator", async () = assert.equal(formatted, "SELECT a -- c\n;"); }); -test("formats a comment on its own line without extra breaks", async () => { +test("keeps a source trailing comment on the field line", async () => { const formatted = await formatSqlText("select a, -- c\nb from t", "postgres", { keywordCase: "upper" }); - assert.equal(formatted, "SELECT a,\n -- c\n b\nFROM t"); + assert.equal(formatted, "SELECT a, -- c\n b\nFROM t"); }); test("compressSqlText collapses whitespace into single spaces", () => {