Accept silent spaces and bar separators, and report unknown commands

docs/Makefile

@@ -43,7 +43,7 @@ gh-pages: build
 		git add -A && \
 		git commit -m "Deploy to GitHub Pages" && \
 		git branch -M gh-pages && \
-		git remote add origin git@panyam-github:panyam/notations.git && \
+		git remote add origin git@github.com:panyam/notations.git && \
 		git push -f origin gh-pages
 	@echo "Deployed! Enable GitHub Pages in repo settings to serve from gh-pages branch"
 	@rm -rf dist/docs/.git

docs/content/reference/syntax/index.html

@@ -54,6 +54,22 @@ <h3>Whitespace</h3>
 
 <p>Both lines produce identical output.</p>
 
+<h3>Bar Separators</h3>
+
+<p>A <code>|</code> or <code>||</code> inside a role line is ignored the same way whitespace is, so
+you can mark avartanam boundaries in the source if you find them easier to read. The renderer draws
+its own lines from the cycle either way, one at the end of each bar and a double line at the end of
+the avartanam, so a bar you type is never what you see:</p>
+
+<notation id="bar-separators" showSource="true" caption="Bar Separators">
+\cycle("|4|2|2|")
+Sw: S R G M | P D | N S. ||
+</notation>
+
+<p>Because they are ignored rather than checked, a bar in the wrong place is not an error. The
+<code>|</code> characters inside <code>\cycle("|4|2|2|")</code> are a different thing entirely,
+since that argument is a quoted string and the bars in it define the cycle.</p>
+
 <h3>Spaces (Karvais)</h3>
 
 <p>Spaces represent empty durations where the previous note continues. Use commas:</p>
@@ -69,15 +85,17 @@ <h3>Spaces (Karvais)</h3>
   <tbody>
     <tr><td><code>,</code></td><td>Space (Karvai)</td><td>1 unit</td></tr>
     <tr><td><code>;</code></td><td>Double space</td><td>2 units</td></tr>
-    <tr><td><code>_</code></td><td>Silent space (not implemented yet)</td><td>1 unit (not displayed)</td></tr>
+    <tr><td><code>_</code></td><td>Silent space</td><td>1 unit (not displayed)</td></tr>
   </tbody>
 </table>
 
 <notation id="spaces" showSource="true" caption="Spaces">
 S , R ; G , , M
 </notation>
 
-<p>Silent spaces are meant to consume duration without displaying the space symbol, to avoid clutter. They are not implemented yet, so use <code>,</code> for now.</p>
+<p>Silent spaces consume duration without displaying the space symbol, to avoid clutter. Only a
+standalone <code>_</code> counts as one, so write <code>_ _ _</code> for three. A run such as
+<code>___</code> is an ordinary literal, as is an underscore inside a word.</p>
 
 <h3>Rests</h3>
 

docs/tests/playground-patterns.spec.ts

@@ -152,12 +152,10 @@ describe("Playground Patterns", () => {
       expect(addAtomsCmd!.atoms.length).toBe(3);
     });
 
-    it("should throw on invalid commands", () => {
+    it("should report invalid commands as errors rather than throwing", () => {
       const parser = new Parser();
-      // Invalid commands throw an error
-      expect(() => {
-        parser.parse("S R G \\invalidCommand");
-      }).toThrow("Invalid command");
+      parser.parse("S R G \\invalidCommand");
+      expect(parser.errors.map((e: any) => e.message)).toEqual(["Invalid command: invalidcommand"]);
     });
   });
 });

src/parser.ts

@@ -67,11 +67,13 @@ const [parser, tokenFunc /*itemGraph*/] = G.newParser(
     %token  DOTS_IDENT    /(\.+)({IdentChar}+)/     { toOctavedNote   }
     %token  IDENT_DOTS    /({IdentChar}+)(\.+)/     { toOctavedNote   }
     %token  IDENT_COLON   /{IdentChar}+:/           { toRoleSelector  }
+    %token  UNDER_SCORE   /_(?!{IdentChar})/
     %token  IDENT         /{IdentChar}+/
     %token  BSLASH_IDENT  /\\{IDENT}/               { toCommandName   }
     %token  BSLASH_NUMBER /\\{NUMBER}/
     %token  HYPHEN        /-/
     %skip                 /[ \t\n\f\r]+/
+    %skip                 /\|\|?/
     %skip                 /\/\/[^\n]*/
     %skip                 /\/\*.*?\*\//
 
@@ -477,6 +479,9 @@ export class Parser {
       const blockCommands = children[2].value as Command[] | null;
 
       const innerCommand = this.createCommand(name, params);
+      // The block's commands were already spliced out of this.commands by endBlock, so an unknown
+      // command takes its block down with it rather than leaving those commands at the top level.
+      if (innerCommand === null) return null;
 
       if (blockCommands !== null) {
         // Wrap in BlockCommand
@@ -503,8 +508,8 @@ export class Parser {
       return children[1].value;
     },
     appendCommand: (rule: G.Rule, parent: G.PTNode, ...children: G.PTNode[]) => {
-      const command = children[1].value as Command;
-      this.addCommand(command);
+      const command = children[1].value as Command | null;
+      if (command !== null) this.addCommand(command);
 
       const atoms = children[2].value as Atom[];
       if (atoms.length > 0) {
@@ -540,7 +545,16 @@ export class Parser {
     // config = config || {};
   }
 
-  createCommand(name: string, params: CmdParam[]): Command {
+  /**
+   * Builds the command a `\\name(...)` directive names.
+   *
+   * Returns null for a name with no command behind it, after recording a ParseError on this
+   * parser, rather than throwing. Rule handlers run inside galore's parse loop, so a throw
+   * escapes `parse` and `load` entirely and reaches the caller as an exception while every other
+   * parse failure arrives in the errors array. Callers get one failure shape by being handed a
+   * null here and reading `errors`.
+   */
+  createCommand(name: string, params: CmdParam[]): Command | null {
     const lName = name.trim().toLowerCase();
     params = params || [];
     if (lName == "line") {
@@ -564,7 +578,8 @@ export class Parser {
       return new Repeat(params);
     } else {
       // Try to set this as the current role
-      throw new Error("Invalid command: " + lName);
+      this.errors.push(new G.ParseError("Invalid command: " + lName, "InvalidCommand", lName));
+      return null;
     }
   }
 

src/tests/parser.spec.ts

@@ -977,3 +977,98 @@ describe("Comment Tests", () => {
     ]);
   });
 });
+
+describe("Silent Space Tests", () => {
+  function atomsOf(input: string): any[] {
+    const parser = new Parser();
+    parser.parse(input);
+    expect(parser.errors).toEqual([]);
+    const addAtoms = parser.commands.map((c: any) => c.debugValue()).find((c: any) => c.atoms);
+    return addAtoms.atoms;
+  }
+
+  test("A lone underscore is a silent space", () => {
+    expect(atomsOf(`Sw: S _ R`)).toEqual([
+      { type: "Literal", value: "S" },
+      { type: "Space", isSilent: true },
+      { type: "Literal", value: "R" },
+    ]);
+  });
+
+  test("Separated underscores are one silent space each", () => {
+    expect(atomsOf(`Sw: _ _ _`)).toEqual([
+      { type: "Space", isSilent: true },
+      { type: "Space", isSilent: true },
+      { type: "Space", isSilent: true },
+    ]);
+  });
+
+  test("A comma is a space that is not silent", () => {
+    expect(atomsOf(`Sw: S , R`)).toEqual([
+      { type: "Literal", value: "S" },
+      { type: "Space", isSilent: false },
+      { type: "Literal", value: "R" },
+    ]);
+  });
+
+  // The token is guarded by a lookahead so that only a standalone underscore becomes a space.
+  // Runs and embedded underscores stay literals, which is what they were before the token existed.
+  test("An underscore run stays a literal", () => {
+    expect(atomsOf(`Sw: S ___ R`)).toEqual([
+      { type: "Literal", value: "S" },
+      { type: "Literal", value: "___" },
+      { type: "Literal", value: "R" },
+    ]);
+  });
+
+  test("An underscore inside a word stays part of the word", () => {
+    expect(atomsOf(`Sw: S_N_D _abc`)).toEqual([
+      { type: "Literal", value: "S_N_D" },
+      { type: "Literal", value: "_abc" },
+    ]);
+  });
+
+  test("Inside a group a silent space differs from a comma only by isSilent", () => {
+    const silent = atomsOf(`Sw: [S _ R]`);
+    const comma = atomsOf(`Sw: [S , R]`);
+    expect(silent[0].atoms[1]).toEqual({ type: "Space", isSilent: true });
+    expect(comma[0].atoms[1]).toEqual({ type: "Space", isSilent: false });
+    expect({ ...silent[0], atoms: null }).toEqual({ ...comma[0], atoms: null });
+  });
+});
+
+describe("Bar Separator Tests", () => {
+  function parseOf(input: string): Parser {
+    const parser = new Parser();
+    parser.parse(input);
+    return parser;
+  }
+
+  function expectSameAsUnbarred(barred: string, unbarred: string): void {
+    const withBars = parseOf(barred);
+    expect(withBars.errors).toEqual([]);
+    expect(withBars.commands.map((c: any) => c.debugValue())).toEqual(
+      parseOf(unbarred).commands.map((c: any) => c.debugValue()),
+    );
+  }
+
+  test("Single and double bars in a role line are ignored", () => {
+    expectSameAsUnbarred(`Sw: S R G M | P D | N S. ||`, `Sw: S R G M P D N S.`);
+  });
+
+  test("A bar with no surrounding whitespace is ignored", () => {
+    expectSameAsUnbarred(`Sw: S R|G M`, `Sw: S R G M`);
+  });
+
+  test("A bar between commands is ignored", () => {
+    expectSameAsUnbarred(`\\beatDuration(2) | Sw: S R`, `\\beatDuration(2) Sw: S R`);
+  });
+
+  // The skip rule must not reach inside a quoted string, which is where the cycle spec lives.
+  test("Bars inside the cycle spec still define the bars", () => {
+    const parser = parseOf(`\\cycle("|4|2|2|")\nSw: S R G M | P D | N S. ||`);
+    expect(parser.errors).toEqual([]);
+    const cycle = parser.commands[0].debugValue().params[0].value;
+    expect(cycle).toEqual("|4|2|2|");
+  });
+});

src/tests/regressions.spec.ts

@@ -68,3 +68,31 @@ describe("Parser State Isolation", () => {
     ]);
   });
 });
+
+describe("Invalid Commands", () => {
+  const BAD_COMMAND = `\\cycle("|4|")\nSw: S R G M\n\\nosuchcommand\n`;
+
+  test("An unknown command is reported rather than thrown", () => {
+    const [, , errors] = load(BAD_COMMAND);
+    expect(errors.map((e: any) => e.message)).toEqual(["Invalid command: nosuchcommand"]);
+  });
+
+  test("Parsing continues past an unknown command", () => {
+    const parser = new Parser();
+    parser.parse(BAD_COMMAND);
+    const names = parser.commands.map((c: any) => c.debugValue().name);
+    expect(names).toEqual(["SetCycle", "ActivateRole", "AddAtoms"]);
+  });
+
+  test("An unknown command carrying a block drops the block without crashing", () => {
+    const parser = new Parser();
+    parser.parse(`\\cycle("|2|")\n\\nosuchcommand {\n mrid: tham ,\n}\nmrid: thi ,\n`);
+    expect(parser.errors.map((e: any) => e.message)).toEqual(["Invalid command: nosuchcommand"]);
+    expect(parser.commands.map((c: any) => c.debugValue().name)).toEqual(["SetCycle", "ActivateRole", "AddAtoms"]);
+  });
+
+  test("A later load is unaffected by an earlier invalid command", () => {
+    expect(load(BAD_COMMAND)[2].length).toBeGreaterThan(0);
+    expect(load(GOOD)[2]).toEqual([]);
+  });
+});