Repository: sql-formatter-org/sql-formatter
Pull request #376 (2022-08-06 15:36:52)

Fix semicolon-on-separate-line bug
Add `WS.NO_NEWLINE` and `WS.MANDATORY_NEWLINE` to Layout engine. Also cover Layout class with tests.

Use these new features to:

1. Eliminate trailing newline before inserting semicolon.
2. Add non-removable newline at the end of a line-comment.


Patch:
diff --git a/src/formatter/ExpressionFormatter.ts b/src/formatter/ExpressionFormatter.ts
index 93913b26..92de94d5 100644
--- a/src/formatter/ExpressionFormatter.ts
+++ b/src/formatter/ExpressionFormatter.ts
@@ -231,7 +231,7 @@ export default class ExpressionFormatter {
 
   /** Formats a line comment onto query */
   private formatLineComment(token: Token) {
-    this.layout.add(this.show(token), WS.NEWLINE, WS.INDENT);
+    this.layout.add(this.show(token), WS.MANDATORY_NEWLINE, WS.INDENT);
   }
 
   /** Formats a block comment onto query */
diff --git a/src/formatter/Formatter.ts b/src/formatter/Formatter.ts
index 7300c2b3..bfda7ba5 100644
--- a/src/formatter/Formatter.ts
+++ b/src/formatter/Formatter.ts
@@ -72,7 +72,7 @@ export default class Formatter {
     } else if (this.cfg.newlineBeforeSemicolon) {
       layout.add(WS.NEWLINE, ';');
     } else {
-      layout.add(WS.NO_SPACE, ';');
+      layout.add(WS.NO_NEWLINE, ';');
     }
     return layout.toString();
   }
diff --git a/src/formatter/Layout.ts b/src/formatter/Layout.ts
index a364912e..5173f7ce 100644
--- a/src/formatter/Layout.ts
+++ b/src/formatter/Layout.ts
@@ -6,12 +6,14 @@ import Indentation from './Indentation';
 export enum WS {
   SPACE, // Adds single space
   NO_SPACE, // Removes preceding horizontal whitespace (if any)
+  NO_NEWLINE, // Removes all preceding whitespace (whether horizontal or vertical)
   NEWLINE, // Adds single newline (and removes any preceding whitespace)
+  MANDATORY_NEWLINE, // Adds single newline that can't be removed by NO_NEWLINE
   INDENT, // Adds indentation (as much as needed for current indentation level)
   SINGLE_INDENT, // Adds whitespace for single indentation step
 }
 
-export type LayoutItem = WS.SPACE | WS.SINGLE_INDENT | WS.NEWLINE | string;
+export type LayoutItem = WS.SPACE | WS.SINGLE_INDENT | WS.NEWLINE | WS.MANDATORY_NEWLINE | string;
 
 /**
  * API for constructing SQL string (especially the whitespace part).
@@ -37,9 +39,16 @@ export default class Layout {
         case WS.NO_SPACE:
           this.trimHorizontalWhitespace();
           break;
+        case WS.NO_NEWLINE:
+          this.trimWhitespace();
+          break;
         case WS.NEWLINE:
           this.trimHorizontalWhitespace();
-          this.addNewline();
+          this.addNewline(WS.NEWLINE);
+          break;
+        case WS.MANDATORY_NEWLINE:
+          this.trimHorizontalWhitespace();
+          this.addNewline(WS.MANDATORY_NEWLINE);
           break;
         case WS.INDENT:
           this.addIndentation();
@@ -59,9 +68,26 @@ export default class Layout {
     }
   }
 
-  private addNewline() {
-    if (this.items.length > 0 && last(this.items) !== WS.NEWLINE) {
-      this.items.push(WS.NEWLINE);
+  private trimWhitespace() {
+    while (isRemovableWhitespace(last(this.items))) {
+      this.items.pop();
+    }
+  }
+
+  private addNewline(newline: WS.NEWLINE | WS.MANDATORY_NEWLINE) {
+    if (this.items.length > 0) {
+      switch (last(this.items)) {
+        case WS.NEWLINE:
+          this.items.pop();
+          this.items.push(newline);
+          break;
+        case WS.MANDATORY_NEWLINE:
+          // keep as is
+          break;
+        default:
+          this.items.push(newline);
+          break;
+      }
     }
   }
 
@@ -83,6 +109,7 @@ export default class Layout {
       case WS.SPACE:
         return ' ';
       case WS.NEWLINE:
+      case WS.MANDATORY_NEWLINE:
         return '\n';
       case WS.SINGLE_INDENT:
         return this.indentation.getSingleIndent();
@@ -94,3 +121,6 @@ export default class Layout {
 
 const isHorizontalWhitespace = (item: WS | string | undefined) =>
   item === WS.SPACE || item === WS.SINGLE_INDENT;
+
+const isRemovableWhitespace = (item: WS | string | undefined) =>
+  item === WS.SPACE || item === WS.SINGLE_INDENT || item === WS.NEWLINE;


Test patch:
diff --git a/test/behavesLikeMariaDbFormatter.ts b/test/behavesLikeMariaDbFormatter.ts
index 0f62b363..15323aec 100644
--- a/test/behavesLikeMariaDbFormatter.ts
+++ b/test/behavesLikeMariaDbFormatter.ts
@@ -127,7 +127,7 @@ export default function behavesLikeMariaDbFormatter(format: FormatFn) {
     expect(
       format(
         `ALTER TABLE t ALTER COLUMN foo SET DEFAULT 10;
-         ALTER TABLE t ALTER COLUMN foo DROP DEFAULT`
+         ALTER TABLE t ALTER COLUMN foo DROP DEFAULT;`
       )
     ).toBe(dedent`
       ALTER TABLE
@@ -141,7 +141,7 @@ export default function behavesLikeMariaDbFormatter(format: FormatFn) {
         t
       ALTER COLUMN
         foo
-      DROP DEFAULT
+      DROP DEFAULT;
     `);
   });
 }
diff --git a/test/db2.test.ts b/test/db2.test.ts
index dd78c55c..9f90ccfa 100644
--- a/test/db2.test.ts
+++ b/test/db2.test.ts
@@ -115,7 +115,7 @@ describe('Db2Formatter', () => {
     expect(
       format(
         `ALTER TABLE t ALTER COLUMN foo SET DATA TYPE VARCHAR;
-         ALTER TABLE t ALTER COLUMN foo SET NOT NULL`
+         ALTER TABLE t ALTER COLUMN foo SET NOT NULL;`
       )
     ).toBe(dedent`
       ALTER TABLE
@@ -129,7 +129,7 @@ describe('Db2Formatter', () => {
         t
       ALTER COLUMN
         foo
-      SET NOT NULL
+      SET NOT NULL;
     `);
   });
 });
diff --git a/test/options/newlineBeforeSemicolon.ts b/test/options/newlineBeforeSemicolon.ts
index 37058614..79ffc76a 100644
--- a/test/options/newlineBeforeSemicolon.ts
+++ b/test/options/newlineBeforeSemicolon.ts
@@ -21,6 +21,25 @@ export default function supportsNewlineBeforeSemicolon(format: FormatFn) {
     `);
   });
 
+  // A regression test for semicolon placement in single-line clauses like:
+  //
+  //   ALTER TABLE
+  //     my_table
+  //   ALTER COLUMN
+  //     foo
+  //   DROP DEFAULT;  <-- here
+  //
+  // Unfortunately there's really no such single-line clause that exists in all dialects,
+  // so our test resorts to using somewhat invalid SQL.
+  it('places semicolon on the same line as a single-line clause', () => {
+    const result = format(`SELECT a FROM;`);
+    expect(result).toBe(dedent`
+      SELECT
+        a
+      FROM;
+    `);
+  });
+
   it('supports semicolon on separate line', () => {
     const result = format(`SELECT a FROM b;`, { newlineBeforeSemicolon: true });
     expect(result).toBe(dedent`
diff --git a/test/postgresql.test.ts b/test/postgresql.test.ts
index 68caee62..23ca2585 100644
--- a/test/postgresql.test.ts
+++ b/test/postgresql.test.ts
@@ -132,7 +132,7 @@ describe('PostgreSqlFormatter', () => {
          ALTER TABLE t ALTER COLUMN foo SET DEFAULT 5;
          ALTER TABLE t ALTER COLUMN foo DROP DEFAULT;
          ALTER TABLE t ALTER COLUMN foo SET NOT NULL;
-         ALTER TABLE t ALTER COLUMN foo DROP NOT NULL`
+         ALTER TABLE t ALTER COLUMN foo DROP NOT NULL;`
       )
     ).toBe(dedent`
       ALTER TABLE
@@ -153,21 +153,19 @@ describe('PostgreSqlFormatter', () => {
         t
       ALTER COLUMN
         foo
-      DROP DEFAULT
-      ;
+      DROP DEFAULT;
 
       ALTER TABLE
         t
       ALTER COLUMN
         foo
-      SET NOT NULL
-      ;
+      SET NOT NULL;
 
       ALTER TABLE
         t
       ALTER COLUMN
         foo
-      DROP NOT NULL
+      DROP NOT NULL;
     `);
   });
 });
diff --git a/test/sql.test.ts b/test/sql.test.ts
index 7781cf36..aae81c0a 100644
--- a/test/sql.test.ts
+++ b/test/sql.test.ts
@@ -91,15 +91,13 @@ describe('SqlFormatter', () => {
         t
       ALTER COLUMN
         foo
-      DROP DEFAULT
-      ;
+      DROP DEFAULT;
 
       ALTER TABLE
         t
       ALTER COLUMN
         foo
-      DROP SCOPE CASCADE
-      ;
+      DROP SCOPE CASCADE;
 
       ALTER TABLE
         t
diff --git a/test/unit/Layout.test.ts b/test/unit/Layout.test.ts
new file mode 100644
index 00000000..539339da
--- /dev/null
+++ b/test/unit/Layout.test.ts
@@ -0,0 +1,130 @@
+import Indentation from 'src/formatter/Indentation';
+import Layout, { WS } from 'src/formatter/Layout';
+
+describe('Layout', () => {
+  function testLayout(...items: (WS | string)[]): string {
+    // an Indentation object with two steps of indentation
+    const indentation = new Indentation('-->');
+    indentation.increaseTopLevel();
+    indentation.increaseTopLevel();
+
+    const layout = new Layout(indentation);
+    layout.add(...items);
+    return layout.toString();
+  }
+
+  it('simply concatenates plain strings', () => {
+    expect(testLayout('hello', 'world')).toBe('helloworld');
+  });
+
+  describe('WS.SPACE', () => {
+    it('inserts single space', () => {
+      expect(testLayout('hello', WS.SPACE, 'world')).toBe('hello world');
+    });
+  });
+
+  describe('WS.SINGLE_INDENT', () => {
+    it('inserts single indentation step', () => {
+      expect(testLayout('hello', WS.NEWLINE, WS.SINGLE_INDENT, 'world')).toBe('hello\n-->world');
+    });
+
+    it('inserts two indentation steps when used twice', () => {
+      expect(testLayout('hello', WS.NEWLINE, WS.SINGLE_INDENT, WS.SINGLE_INDENT, 'world')).toBe(
+        'hello\n-->-->world'
+      );
+    });
+  });
+
+  describe('WS.INDENT', () => {
+    it('inserts current amount of indentation', () => {
+      expect(testLayout('hello', WS.NEWLINE, WS.INDENT, 'world')).toBe('hello\n-->-->world');
+    });
+
+    it('inserts double the current indentation when used twice', () => {
+      expect(testLayout('hello', WS.NEWLINE, WS.INDENT, WS.INDENT, 'world')).toBe(
+        'hello\n-->-->-->-->world'
+      );
+    });
+  });
+
+  describe('WS.NO_SPACE', () => {
+    it('does nothing when no preceding whitespace', () => {
+      expect(testLayout('hello', WS.NO_SPACE, 'world')).toBe('helloworld');
+    });
+
+    it('removes all preceding spaces', () => {
+      expect(testLayout('hello', WS.SPACE, WS.SPACE, WS.NO_SPACE, 'world')).toBe('helloworld');
+    });
+
+    it('removes all preceding indentation', () => {
+      expect(
+        testLayout('hello', WS.NEWLINE, WS.SINGLE_INDENT, WS.SINGLE_INDENT, WS.NO_SPACE, 'world')
+      ).toBe('hello\nworld');
+      expect(testLayout('hello', WS.NEWLINE, WS.INDENT, WS.NO_SPACE, 'world')).toBe('hello\nworld');
+    });
+
+    it('does not remove preceding newline', () => {
+      expect(testLayout('hello', WS.NEWLINE, WS.NO_SPACE, 'world')).toBe('hello\nworld');
+      expect(testLayout('hello', WS.MANDATORY_NEWLINE, WS.NO_SPACE, 'world')).toBe('hello\nworld');
+    });
+
+    it('removes preceding spaces up to first newline', () => {
+      expect(testLayout('hello', WS.NEWLINE, WS.SPACE, WS.NO_SPACE, 'world')).toBe('hello\nworld');
+      expect(testLayout('hello', WS.MANDATORY_NEWLINE, WS.SPACE, WS.NO_SPACE, 'world')).toBe(
+        'hello\nworld'
+      );
+    });
+  });
+
+  describe('WS.NEWLINE', () => {
+    it('inserts single newline', () => {
+      expect(testLayout('hello', WS.NEWLINE, 'world')).toBe('hello\nworld');
+    });
+
+    it('inserts single newline, even when used twice', () => {
+      expect(testLayout('hello', WS.NEWLINE, WS.NEWLINE, 'world')).toBe('hello\nworld');
+    });
+
+    it('trims preceding horizontal whitespace', () => {
+      expect(testLayout('hello', WS.SPACE, WS.NEWLINE, 'world')).toBe('hello\nworld');
+      expect(testLayout('hello', WS.INDENT, WS.NEWLINE, 'world')).toBe('hello\nworld');
+      expect(testLayout('hello', WS.SINGLE_INDENT, WS.NEWLINE, 'world')).toBe('hello\nworld');
+    });
+  });
+
+  describe('WS.MANDATORY_NEWLINE', () => {
+    it('inserts single newline', () => {
+      expect(testLayout('hello', WS.MANDATORY_NEWLINE, 'world')).toBe('hello\nworld');
+    });
+
+    it('inserts single newline, even when used twice', () => {
+      expect(testLayout('hello', WS.MANDATORY_NEWLINE, WS.MANDATORY_NEWLINE, 'world')).toBe(
+        'hello\nworld'
+      );
+    });
+
+    it('inserts single newline, even when used after plain WS.NEWLINE', () => {
+      expect(testLayout('hello', WS.NEWLINE, WS.MANDATORY_NEWLINE, 'world')).toBe('hello\nworld');
+    });
+
+    it('inserts single newline, even when used before plain WS.NEWLINE', () => {
+      expect(testLayout('hello', WS.NEWLINE, WS.MANDATORY_NEWLINE, 'world')).toBe('hello\nworld');
+    });
+  });
+
+  describe('WS.NO_NEWLINE', () => {
+    it('removes all preceding spaces', () => {
+      expect(testLayout('hello', WS.SPACE, WS.NO_NEWLINE, 'world')).toBe('helloworld');
+    });
+
+    it('removes all preceding newlines', () => {
+      expect(testLayout('hello', WS.NEWLINE, WS.NO_NEWLINE, 'world')).toBe('helloworld');
+    });
+
+    it('does not remove preceding mandatory newlines', () => {
+      expect(testLayout('hello', WS.MANDATORY_NEWLINE, WS.NO_NEWLINE, 'world')).toBe(
+        'hello\nworld'
+      );
+    });
+  });
+});