Repository: sql-formatter-org/sql-formatter
Pull request #263 (2022-06-28 16:32:44)

8.0: Restore indentStyle support
Currently having some trouble restoring the original style.

Previously everything was just a stream of tokens, so you could indent things however you pleased, but now the following SQL:

```sql
SELECT *
FROM foo JOIN bar
```

gets parsed into structure like:

```js
[
  {type: "clause", name: "SELECT", children: ["*"]},
  {type: "clause", name: "FROM", children: ["foo", "JOIN", "bar"]},
]
```

and each array of children gets formatted in isolation, assuming that they are all indented 1 step more than the preceding `SELECT` or `FROM`. But that doesn't hold true for the formatting of `JOIN` in tabular style - there we would want to indent it at the same level as `SELECT`.

I see a few ways here:

- change the Parser to output JOIN blocks to the same level as clauses. Don't really like that. The AST shouldn't depend  on our formatting needs.
- change the formatting logic to eliminate this limit. Like instead of re-indenting the formatted sub-expression, passing the current indentation to the sub-expression formatter.



Patch:
diff --git a/src/core/ExpressionFormatter.ts b/src/core/ExpressionFormatter.ts
index f330c760..088bac23 100644
--- a/src/core/ExpressionFormatter.ts
+++ b/src/core/ExpressionFormatter.ts
@@ -1,7 +1,6 @@
 import type { FormatOptions } from 'src/types';
 import { equalizeWhitespace } from 'src/utils';
 
-import Indentation from './Indentation';
 import InlineBlock from './InlineBlock';
 import Params from './Params';
 import { isReserved, type Token, TokenType } from './token';
@@ -16,13 +15,20 @@ import {
   LimitClause,
   Parenthesis,
 } from './ast';
-import { indentString } from './config';
-import WhitespaceBuilder, { LayoutItem, WS } from './WhitespaceBuilder';
+import { isTabularStyle } from './config';
+import WhitespaceBuilder, { WS } from './WhitespaceBuilder';
+import toTabularFormat, { isTabularToken } from './tabularStyle';
+
+interface ExpressionFormatterParams {
+  cfg: FormatOptions;
+  params: Params;
+  query: WhitespaceBuilder;
+  inline?: boolean;
+}
 
 /** Formats a generic SQL expression */
 export default class ExpressionFormatter {
   private cfg: FormatOptions;
-  private indentation: Indentation;
   private inlineBlock: InlineBlock;
   private params: Params;
   private query: WhitespaceBuilder;
@@ -31,13 +37,12 @@ export default class ExpressionFormatter {
   private nodes: AstNode[] = [];
   private index = -1;
 
-  constructor(cfg: FormatOptions, params: Params, { inline = false }: { inline?: boolean } = {}) {
+  constructor({ cfg, params, query, inline = false }: ExpressionFormatterParams) {
     this.cfg = cfg;
     this.inline = inline;
-    this.indentation = new Indentation(indentString(cfg));
     this.inlineBlock = new InlineBlock(this.cfg.expressionWidth);
     this.params = params;
-    this.query = new WhitespaceBuilder(this.indentation);
+    this.query = query;
   }
 
   public format(nodes: AstNode[]): WhitespaceBuilder {
@@ -91,16 +96,22 @@ export default class ExpressionFormatter {
   private formatParenthesis(node: Parenthesis) {
     const inline = this.inlineBlock.isInlineBlock(node);
 
-    const subLayout = this.formatSubExpression(node.children, inline);
-
     if (inline) {
-      this.query.add(node.openParen, ...subLayout, WS.NO_SPACE, node.closeParen, WS.SPACE);
+      this.query.add(node.openParen);
+      this.query = this.formatSubExpression(node.children, inline);
+      this.query.add(WS.NO_SPACE, node.closeParen, WS.SPACE);
     } else {
       this.query.add(node.openParen, WS.NEWLINE);
 
-      this.indentation.increaseBlockLevel();
-      this.query.addLayout(subLayout);
-      this.indentation.decreaseBlockLevel();
+      if (isTabularStyle(this.cfg)) {
+        this.query.add(WS.INDENT);
+        this.query = this.formatSubExpression(node.children, inline);
+      } else {
+        this.query.indentation.increaseBlockLevel();
+        this.query.add(WS.INDENT);
+        this.query = this.formatSubExpression(node.children, inline);
+        this.query.indentation.decreaseBlockLevel();
+      }
 
       this.query.add(WS.NEWLINE, WS.INDENT, node.closeParen, WS.SPACE);
     }
@@ -120,28 +131,32 @@ export default class ExpressionFormatter {
   }
 
   private formatClause(node: Clause) {
-    const subLayout = this.formatSubExpression(node.children);
+    if (isTabularStyle(this.cfg)) {
+      this.query.add(WS.NEWLINE, WS.INDENT, this.show(node.nameToken), WS.SPACE);
+    } else {
+      this.query.add(WS.NEWLINE, WS.INDENT, this.show(node.nameToken), WS.NEWLINE);
+    }
+    this.query.indentation.increaseTopLevel();
 
-    this.indentation.decreaseTopLevel();
-    this.query.add(WS.NEWLINE, WS.INDENT, this.show(node.nameToken), WS.NEWLINE);
-    this.indentation.increaseTopLevel();
+    if (!isTabularStyle(this.cfg)) {
+      this.query.add(WS.INDENT);
+    }
+    this.query = this.formatSubExpression(node.children);
 
-    this.query.addLayout(subLayout);
+    this.query.indentation.decreaseTopLevel();
   }
 
   private formatBinaryClause(node: BinaryClause) {
-    const subLayout = this.formatSubExpression(node.children);
-
-    this.indentation.decreaseTopLevel();
+    this.query.indentation.decreaseTopLevel();
     this.query.add(WS.NEWLINE, WS.INDENT, this.show(node.nameToken), WS.NEWLINE);
 
-    this.query.addLayout(subLayout);
+    this.query.add(WS.INDENT);
+    this.query = this.formatSubExpression(node.children);
   }
 
   private formatLimitClause(node: LimitClause) {
-    this.indentation.decreaseTopLevel();
     this.query.add(WS.NEWLINE, WS.INDENT, this.show(node.limitToken));
-    this.indentation.increaseTopLevel();
+    this.query.indentation.increaseTopLevel();
 
     if (node.offsetToken) {
       this.query.add(
@@ -156,6 +171,7 @@ export default class ExpressionFormatter {
     } else {
       this.query.add(WS.NEWLINE, WS.INDENT, this.show(node.countToken), WS.SPACE);
     }
+    this.query.indentation.decreaseTopLevel();
   }
 
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
@@ -163,8 +179,13 @@ export default class ExpressionFormatter {
     this.query.add('*', WS.SPACE);
   }
 
-  private formatSubExpression(nodes: AstNode[], inline = this.inline): LayoutItem[] {
-    return new ExpressionFormatter(this.cfg, this.params, { inline }).format(nodes).toLayout();
+  private formatSubExpression(nodes: AstNode[], inline = this.inline): WhitespaceBuilder {
+    return new ExpressionFormatter({
+      cfg: this.cfg,
+      params: this.params,
+      query: this.query,
+      inline,
+    }).format(nodes);
   }
 
   private formatToken(token: Token): void {
@@ -232,7 +253,14 @@ export default class ExpressionFormatter {
   }
 
   private formatJoin(token: Token) {
-    this.query.add(WS.NEWLINE, WS.INDENT, this.show(token), WS.SPACE);
+    if (isTabularStyle(this.cfg)) {
+      // in tabular style JOINs are at the same level as clauses
+      this.query.indentation.decreaseTopLevel();
+      this.query.add(WS.NEWLINE, WS.INDENT, this.show(token), WS.SPACE);
+      this.query.indentation.increaseTopLevel();
+    } else {
+      this.query.add(WS.NEWLINE, WS.INDENT, this.show(token), WS.SPACE);
+    }
   }
 
   /**
@@ -283,14 +311,21 @@ export default class ExpressionFormatter {
    */
   private formatLogicalOperator(token: Token) {
     if (this.cfg.logicalOperatorNewline === 'before') {
-      this.query.add(WS.NEWLINE, WS.INDENT, this.show(token), WS.SPACE);
+      if (isTabularStyle(this.cfg)) {
+        // In tabular style AND/OR is placed on the same level as clauses
+        this.query.indentation.decreaseTopLevel();
+        this.query.add(WS.NEWLINE, WS.INDENT, this.show(token), WS.SPACE);
+        this.query.indentation.increaseTopLevel();
+      } else {
+        this.query.add(WS.NEWLINE, WS.INDENT, this.show(token), WS.SPACE);
+      }
     } else {
       this.query.add(this.show(token), WS.NEWLINE, WS.INDENT);
     }
   }
 
   private formatCaseStart(token: Token) {
-    this.indentation.increaseBlockLevel();
+    this.query.indentation.increaseBlockLevel();
     this.query.add(this.show(token), WS.NEWLINE, WS.INDENT);
   }
 
@@ -299,7 +334,7 @@ export default class ExpressionFormatter {
   }
 
   private formatMultilineBlockEnd(token: Token) {
-    this.indentation.decreaseBlockLevel();
+    this.query.indentation.decreaseBlockLevel();
 
     this.query.add(WS.NEWLINE, WS.INDENT, this.show(token), WS.SPACE);
   }
@@ -323,7 +358,11 @@ export default class ExpressionFormatter {
   }
 
   private show(token: Token): string {
-    return this.showToken(token);
+    if (isTabularToken(token)) {
+      return toTabularFormat(this.showToken(token), this.cfg.indentStyle);
+    } else {
+      return this.showToken(token);
+    }
   }
 
   // don't call this directly, always use show() instead.
diff --git a/src/core/Formatter.ts b/src/core/Formatter.ts
index e24daaf3..f7fdd1be 100644
--- a/src/core/Formatter.ts
+++ b/src/core/Formatter.ts
@@ -8,7 +8,8 @@ import ExpressionFormatter from './ExpressionFormatter';
 import { indentString } from './config';
 import AliasAs from './AliasAs';
 import { Statement } from './ast';
-import { WS } from './WhitespaceBuilder';
+import WhitespaceBuilder, { WS } from './WhitespaceBuilder';
+import Indentation from './Indentation';
 
 /** Main formatter class that produces a final output string from list of tokens */
 export default class Formatter {
@@ -60,7 +61,12 @@ export default class Formatter {
   }
 
   private formatStatement(statement: Statement): string {
-    const wsBuilder = new ExpressionFormatter(this.cfg, this.params).format(statement.children);
+    const wsBuilder = new ExpressionFormatter({
+      cfg: this.cfg,
+      params: this.params,
+      query: new WhitespaceBuilder(new Indentation(indentString(this.cfg))),
+    }).format(statement.children);
+
     if (!statement.hasSemicolon) {
       // do nothing
     } else if (this.cfg.newlineBeforeSemicolon) {
diff --git a/src/core/WhitespaceBuilder.ts b/src/core/WhitespaceBuilder.ts
index 19f18698..9b3670cd 100644
--- a/src/core/WhitespaceBuilder.ts
+++ b/src/core/WhitespaceBuilder.ts
@@ -24,21 +24,7 @@ export type LayoutItem = WS.SPACE | WS.SINGLE_INDENT | WS.NEWLINE | string;
 export default class WhitespaceBuilder {
   private layout: LayoutItem[] = [];
 
-  constructor(private indentation: Indentation) {}
-
-  /**
-   * Appands an already constructed sub-layout
-   * and indents each line in it by current level of indentation
-   */
-  public addLayout(layout: LayoutItem[]) {
-    this.addIndentation();
-    layout.forEach((item, i) => {
-      this.layout.push(item);
-      if (item === WS.NEWLINE && i < layout.length - 1) {
-        this.addIndentation();
-      }
-    });
-  }
+  constructor(public indentation: Indentation) {}
 
   /**
    * Appends token strings and whitespace modifications to SQL string.
@@ -102,13 +88,6 @@ export default class WhitespaceBuilder {
     return this.layout.map(item => this.itemToString(item)).join('');
   }
 
-  /**
-   * Returns the constructed whitespace layout structure.
-   */
-  public toLayout(): LayoutItem[] {
-    return this.layout;
-  }
-
   private itemToString(item: LayoutItem): string {
     switch (item) {
       case WS.SPACE:
diff --git a/src/core/tabularStyle.ts b/src/core/tabularStyle.ts
index b0057382..230fb9d5 100644
--- a/src/core/tabularStyle.ts
+++ b/src/core/tabularStyle.ts
@@ -1,4 +1,5 @@
 import type { IndentStyle } from 'src/types';
+import { Token, TokenType } from './token';
 
 /**
  * When tabular style enabled,
@@ -23,3 +24,16 @@ export default function toTabularFormat(tokenText: string, indentStyle: IndentSt
 
   return tokenText + ['', ...tail].join(' ');
 }
+
+/**
+ * True when the token can be formatted in tabular style
+ */
+export function isTabularToken(token: Token): boolean {
+  return (
+    token.type === TokenType.RESERVED_LOGICAL_OPERATOR ||
+    token.type === TokenType.RESERVED_DEPENDENT_CLAUSE ||
+    token.type === TokenType.RESERVED_COMMAND ||
+    token.type === TokenType.RESERVED_BINARY_COMMAND ||
+    token.type === TokenType.RESERVED_JOIN
+  );
+}


Test patch:
diff --git a/test/behavesLikeSqlFormatter.ts b/test/behavesLikeSqlFormatter.ts
index 9b19ac51..82ccaf33 100644
--- a/test/behavesLikeSqlFormatter.ts
+++ b/test/behavesLikeSqlFormatter.ts
@@ -9,6 +9,7 @@ import supportsUseTabs from './options/useTabs';
 import supportsAliasAs from './options/aliasAs';
 import supportsExpressionWidth from './options/expressionWidth';
 import supportsKeywordCase from './options/keywordCase';
+import supportsIndentStyle from './options/indentStyle';
 import supportsCommaPosition from './options/commaPosition';
 import supportsLinesBetweenQueries from './options/linesBetweenQueries';
 import supportsNewlineBeforeSemicolon from './options/newlineBeforeSemicolon';
@@ -27,7 +28,7 @@ export default function behavesLikeSqlFormatter(format: FormatFn) {
   supportsTabWidth(format);
   supportsUseTabs(format);
   supportsKeywordCase(format);
-  // supportsIndentStyle(format); // XXX: Feature is disabled for now
+  supportsIndentStyle(format);
   supportsLinesBetweenQueries(format);
   // supportsMultilineLists(format); // XXX: Feature is disabled for now
   supportsExpressionWidth(format);
diff --git a/test/features/comments.ts b/test/features/comments.ts
index 68f6dcd5..c6d66360 100644
--- a/test/features/comments.ts
+++ b/test/features/comments.ts
@@ -81,7 +81,7 @@ export default function supportsComments(format: FormatFn, opts: CommentsConfig
     ).toBe(dedent`
       SELECT
         a --comment
-        ,
+      ,
         b
     `);
   });
diff --git a/test/features/operators.ts b/test/features/operators.ts
index a693571e..22edd3a4 100644
--- a/test/features/operators.ts
+++ b/test/features/operators.ts
@@ -20,7 +20,7 @@ export default function supportsOperators(
   });
 
   logicalOperators.forEach(op => {
-    describe(`supports ${op} operator`, () => {
+    it(`supports ${op} operator`, () => {
       const result = format(`SELECT true ${op} false AS foo;`);
       expect(result).toBe(dedent`
         SELECT
diff --git a/test/n1ql.test.ts b/test/n1ql.test.ts
index f36ca5e4..b2adb6c4 100644
--- a/test/n1ql.test.ts
+++ b/test/n1ql.test.ts
@@ -101,7 +101,7 @@ describe('N1qlFormatter', () => {
         'Elinor_33313792'
       NEST
         orders_with_users orders ON KEYS ARRAY s.order_id FOR s IN usr.shipped_order_history
-        END;
+      END;
     `);
   });
 
diff --git a/test/options/indentStyle.ts b/test/options/indentStyle.ts
index 7870b315..4eaba09f 100644
--- a/test/options/indentStyle.ts
+++ b/test/options/indentStyle.ts
@@ -77,8 +77,7 @@ export default function supportsIndentStyle(format: FormatFn) {
       `);
     });
 
-    // TODO: Disabled temporarily
-    it.skip('handles multiple levels of nested queries', () => {
+    it('handles multiple levels of nested queries', () => {
       expect(
         format(
           'SELECT age FROM (SELECT fname, lname, age FROM (SELECT fname, lname FROM persons) JOIN (SELECT age FROM ages)) as mytable;',
diff --git a/test/options/tabulateAlias.ts b/test/options/tabulateAlias.ts
index a9c3aa59..eeb2e29a 100644
--- a/test/options/tabulateAlias.ts
+++ b/test/options/tabulateAlias.ts
@@ -110,7 +110,7 @@ export default function supportsTabulateAlias(format: FormatFn) {
     `);
   });
 
-  it.skip('works together with indentStyle:tabularLeft', () => {
+  it('works together with indentStyle:tabularLeft', () => {
     const result = format(
       dedent`SELECT alpha AS alp, MAX(beta), epsilon AS E FROM ( SELECT mu AS m, iota AS io FROM gamma );`,
       { indentStyle: 'tabularLeft', tabulateAlias: true }
@@ -128,7 +128,7 @@ export default function supportsTabulateAlias(format: FormatFn) {
     `);
   });
 
-  it.skip('works together with indentStyle:tabularRight', () => {
+  it('works together with indentStyle:tabularRight', () => {
     const result = format(
       dedent`SELECT alpha AS alp, MAX(beta), epsilon AS E FROM ( SELECT mu AS m, iota AS io FROM gamma );`,
       { indentStyle: 'tabularRight', tabulateAlias: true }
diff --git a/test/sql.test.ts b/test/sql.test.ts
index 2396fcca..ab591e07 100644
--- a/test/sql.test.ts
+++ b/test/sql.test.ts
@@ -54,7 +54,7 @@ describe('SqlFormatter', () => {
     ).toBe(dedent`
       SELECT
         @ name,
-        : bar
+      : bar
       FROM
         { foo };
     `);