Merge PR #462: Better formatting of inline block-comments

Author: nene

src/formatter/ExpressionFormatter.ts

@@ -1,5 +1,5 @@
 import { FormatOptions } from 'src/FormatOptions';
-import { equalizeWhitespace } from 'src/utils';
+import { equalizeWhitespace, isMultiline } from 'src/utils';
 
 import Params from 'src/formatter/Params';
 import { isTabularStyle } from 'src/formatter/config';
@@ -309,18 +309,26 @@ export default class ExpressionFormatter {
   }
 
   private formatLineComment(node: LineCommentNode) {
-    if (/\n/.test(node.precedingWhitespace || '')) {
+    if (isMultiline(node.precedingWhitespace || '')) {
       this.layout.add(WS.NEWLINE, WS.INDENT, node.text, WS.MANDATORY_NEWLINE, WS.INDENT);
     } else {
       this.layout.add(WS.NO_NEWLINE, WS.SPACE, node.text, WS.MANDATORY_NEWLINE, WS.INDENT);
     }
   }
 
   private formatBlockComment(node: BlockCommentNode) {
-    this.splitBlockComment(node.text).forEach(line => {
-      this.layout.add(WS.NEWLINE, WS.INDENT, line);
-    });
-    this.layout.add(WS.NEWLINE, WS.INDENT);
+    if (this.isMultilineBlockComment(node)) {
+      this.splitBlockComment(node.text).forEach(line => {
+        this.layout.add(WS.NEWLINE, WS.INDENT, line);
+      });
+      this.layout.add(WS.NEWLINE, WS.INDENT);
+    } else {
+      this.layout.add(node.text, WS.SPACE);
+    }
+  }
+
+  private isMultilineBlockComment(node: BlockCommentNode): boolean {
+    return isMultiline(node.text) || isMultiline(node.precedingWhitespace || '');
   }
 
   // Breaks up block comment to multiple lines.

src/parser/ast.ts

@@ -159,6 +159,7 @@ export interface LineCommentNode extends BaseNode {
 export interface BlockCommentNode extends BaseNode {
   type: NodeType.block_comment;
   text: string;
+  precedingWhitespace: string;
 }
 
 export type CommentNode = LineCommentNode | BlockCommentNode;

src/parser/grammar.ne

@@ -308,5 +308,9 @@ comment -> %LINE_COMMENT {%
   })
 %}
 comment -> %BLOCK_COMMENT {%
-  ([token]) => ({ type: NodeType.block_comment, text: token.text })
+  ([token]) => ({
+    type: NodeType.block_comment,
+    text: token.text,
+    precedingWhitespace: token.precedingWhitespace,
+  })
 %}

src/utils.ts

@@ -28,6 +28,9 @@ export const sum = (arr: number[]): number => {
 export const flatKeywordList = (obj: Record<string, string[]>): string[] =>
   dedupe(Object.values(obj).flat());
 
+// True when string contains multiple lines
+export const isMultiline = (text: string): boolean => /\n/.test(text);
+
 // Given a type and a field name, returns a type where this field is optional
 //
 // For example, these two type definitions are equivalent:

test/features/arrayAndMapAccessors.ts

@@ -35,19 +35,15 @@ export default function supportsArrayAndMapAccessors(format: FormatFn) {
     const result = format(`SELECT arr /* comment */ [1];`);
     expect(result).toBe(dedent`
       SELECT
-        arr
-        /* comment */
-        [1];
+        arr/* comment */ [1];
     `);
   });
 
   it('formats namespaced array accessor with comment in-between', () => {
     const result = format(`SELECT foo./* comment */arr[1];`);
     expect(result).toBe(dedent`
       SELECT
-        foo.
-        /* comment */
-        arr[1];
+        foo./* comment */ arr[1];
     `);
   });
 }

test/features/between.ts

@@ -13,13 +13,7 @@ export default function supportsBetween(format: FormatFn) {
   it('formats BETWEEN with comments inside', () => {
     expect(format('WHERE foo BETWEEN /*C1*/ t.bar /*C2*/ AND /*C3*/ t.baz')).toBe(dedent`
       WHERE
-        foo BETWEEN
-        /*C1*/
-        t.bar
-        /*C2*/
-       AND
-        /*C3*/
-        t.baz
+        foo BETWEEN /*C1*/ t.bar /*C2*/ AND /*C3*/ t.baz
     `);
   });
 }

test/features/case.ts

@@ -116,23 +116,10 @@ export default function supportsCase(format: FormatFn) {
 
     expect(result).toBe(dedent`
       SELECT
-        CASE
-        /*c1*/
-        foo
-          /*c2*/
-          WHEN
-          /*c3*/
-          1
-          /*c4*/
-          THEN
-          /*c5*/
-          2
-          /*c6*/
-          ELSE
-          /*c7*/
-          3
-        /*c8*/
-        END;
+        CASE /*c1*/ foo
+          /*c2*/ WHEN /*c3*/ 1 /*c4*/ THEN /*c5*/ 2
+          /*c6*/ ELSE /*c7*/ 3
+        /*c8*/ END;
     `);
   });
 

test/features/comments.ts

@@ -48,6 +48,18 @@ export default function supportsComments(format: FormatFn, opts: CommentsConfig
     expect(format(sql)).toBe(sql);
   });
 
+  it('keeps block comment on separate line when it is separate in original SQL', () => {
+    const sql = dedent`
+      SELECT
+        /* separate-line block comment */
+        foo,
+        bar /* inline block comment */
+      FROM
+        tbl;
+    `;
+    expect(format(sql)).toBe(sql);
+  });
+
   it('formats tricky line comments', () => {
     expect(format('SELECT a--comment, here\nFROM b--comment')).toBe(dedent`
       SELECT
@@ -167,9 +179,7 @@ export default function supportsComments(format: FormatFn, opts: CommentsConfig
     `);
     expect(result).toBe(dedent`
       SELECT
-        count
-        /* comment */
-        (*);
+        count/* comment */ (*);
     `);
   });
 
@@ -179,18 +189,10 @@ export default function supportsComments(format: FormatFn, opts: CommentsConfig
     `);
     expect(result).toBe(dedent`
       SELECT
-        foo
-        /* com1 */
-      .bar,
-        count()
-        /* com2 */
-      .bar,
-        foo.bar
-        /* com3 */
-      .baz,
-        (1, 2)
-        /* com4 */
-      .foo;
+        foo /* com1 */.bar,
+        count() /* com2 */.bar,
+        foo.bar /* com3 */.baz,
+        (1, 2) /* com4 */.foo;
     `);
   });
 
@@ -200,12 +202,8 @@ export default function supportsComments(format: FormatFn, opts: CommentsConfig
     `);
     expect(result).toBe(dedent`
       SELECT
-        foo.
-        /* com1 */
-        bar,
-        foo.
-        /* com2 */
-        *;
+        foo./* com1 */ bar,
+        foo./* com2 */ *;
     `);
   });
 
@@ -226,8 +224,7 @@ export default function supportsComments(format: FormatFn, opts: CommentsConfig
       const result = format('SELECT alpha /* /* commment */ */ FROM beta');
       expect(result).toBe(dedent`
         SELECT
-          alpha
-          /* /* commment */ */
+          alpha /* /* commment */ */
         FROM
           beta
       `);

test/unit/__snapshots__/Parser.test.ts.snap

@@ -362,6 +362,7 @@ Array [
               "text": "my_array",
               "trailingComments": Array [
                 Object {
+                  "precedingWhitespace": " ",
                   "text": "/*haha*/",
                   "type": "block_comment",
                 },