Improves handling of SKIP in formatter (#477)

Author: tomasnyberg

packages/language-support/src/formatting/formatting.ts

@@ -93,6 +93,7 @@ import {
   SetPropContext,
   SetPropsContext,
   ShowCommandYieldContext,
+  SkipContext,
   StatementsOrCommandsContext,
   SubqueryClauseContext,
   TrimFunctionContext,
@@ -847,6 +848,16 @@ export class TreePrintVisitor extends CypherCmdParserVisitor<void> {
     this.endGroup(deleteClauseGrp);
   };
 
+  visitSkip = (ctx: SkipContext) => {
+    const skipGrp = this.startGroup();
+    this._visit(ctx.OFFSET());
+    this._visit(ctx.SKIPROWS());
+    const skipIndent = this.addIndentation();
+    this._visit(ctx.expression());
+    this.removeIndentation(skipIndent);
+    this.endGroup(skipGrp);
+  };
+
   // Separate the return items from the return body so that ORDER BY and LIMIT can be
   // excluded from the whole RETURN group.
   // Used by returnClause and withClause
@@ -863,10 +874,9 @@ export class TreePrintVisitor extends CypherCmdParserVisitor<void> {
   _visitReturnBodyNotItems = (ctx: ReturnBodyContext) => {
     if (ctx.orderBy() || ctx.skip()) {
       this.breakLine();
-      const orderSkipGrp = this.startGroup();
       this._visit(ctx.orderBy());
+      this.breakLine();
       this._visit(ctx.skip());
-      this.endGroup(orderSkipGrp);
     }
     this._visit(ctx.limit());
   };
@@ -888,11 +898,10 @@ export class TreePrintVisitor extends CypherCmdParserVisitor<void> {
     this._visit(ctx.returnItems());
     this.removeIndentation(returnItemsIndent);
     if (ctx.orderBy() || ctx.skip()) {
-      const orderSkipGrp = this.startGroup();
       this.breakLine();
       this._visit(ctx.orderBy());
+      this.breakLine();
       this._visit(ctx.skip());
-      this.endGroup(orderSkipGrp);
     }
     this._visit(ctx.limit());
   };

packages/language-support/src/tests/formatting/linebreaks.test.ts

@@ -1225,6 +1225,38 @@ RETURN count(*)`.trimStart();
     const expected = query;
     verifyFormatting(query, expected);
   });
+
+  test('should keep the ORDER BY parts together', () => {
+    // Right now this works because ORDER BY doesn't have a group at all.
+    // This should be fine since we wouldn't want to break it anyway, but might break if we decide
+    // to start e.g. putting ASC/DESCENDING on a new line and indent them.
+    const query = `MATCH (n)
+WITH n
+ORDER BY n.priiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiice ASC
+LIMIT 100
+RETURN n`;
+    const expected = query;
+    verifyFormatting(query, expected);
+  });
+
+  test('SKIP that needs to break should handle it gracefully', () => {
+    const query = `MATCH (n)
+WITH n
+SKIP CASE WHEN n.department = 'Engineering' THEN 'Engineer' WHEN n.department = 'Sales' THEN 'Sales Leader' ELSE 'Manager' END
+LIMIT 100
+RETURN n`;
+    const expected = `MATCH (n)
+WITH n
+SKIP
+  CASE
+    WHEN n.department = 'Engineering' THEN 'Engineer'
+    WHEN n.department = 'Sales' THEN 'Sales Leader'
+    ELSE 'Manager'
+  END
+LIMIT 100
+RETURN n`;
+    verifyFormatting(query, expected);
+  });
 });
 
 describe('tests for respcecting user line breaks', () => {

packages/language-support/src/tests/formatting/styleguide.test.ts

@@ -346,4 +346,19 @@ CALL (c, c2, trxs, avgTrx, totalSum) {
 } IN 10 CONCURRENT TRANSACTIONS OF 25 ROWS;`;
     verifyFormatting(query, expected);
   });
+
+  test('should put SKIP on a newline, similarly to LIMIT and ORDER BY', () => {
+    const query = `MATCH (n)
+WITH n
+ORDER BY n.price ASC SKIP 10
+LIMIT 100
+RETURN n`;
+    const expected = `MATCH (n)
+WITH n
+ORDER BY n.price ASC
+SKIP 10
+LIMIT 100
+RETURN n`;
+    verifyFormatting(query, expected);
+  });
 });