even more tests

Aerijo · Aerijo · commit 09aa82a7e8e6 · 2020-07-16T22:39:11.000+10:00
diff --git a/lib/snippet-body.pegjs b/lib/snippet-body.pegjs
@@ -97,7 +97,16 @@ regexString = r:([^/\\] / '\\' c:. { return '\\' + c } )* { return r.join(""); }
 
 // The form of a substitution for a transformation. It is a mix of plain text + modifiers + backreferences to the find capture groups
 // It cannot access tab stop values.
-replace = r:(replaceText / format / replaceModifier / escapedReplace / [^}/])* { return coalesce(r); }
+replace = r:(replaceText / format / replaceModifier / escapedReplace / [^/])* { return coalesce(r); }
+
+// Same as replace, but we disallow plain '}' instead of plain '/' because we are inside a format (ended by '}')
+// NOTE: Diallowing escape of '/' is consistent with VS Code. The general rule is "if it's not got a special meaning, it can't be escaped"
+// Inside a format there is no special meaning to '/', so we can't escape it.
+formatReplace = r:(replaceText / format / replaceModifier / escapedFormatReplace / [^}])* { return coalesce(r); }
+
+// Another special case; the if half of an if-else format is terminated by ':'
+ifElseReplace = r:(replaceText / format / replaceModifier / escapedIfElseReplace / [^:])* { return coalesce(r); }
+
 
 // A reference to a capture group of the find regex of a transformation. Can conditionally
 // resolve based on if the match occurred, and have arbitrary modifiers applied to it.
@@ -117,13 +126,16 @@ formatWithModifier = '{' n:integer ':' modifier:modifier '}' { return { backrefe
 // If the `n`th capture group is non-empty, then resolve to the `ifContent` value, else an empty string
 // Note that ifContent is a replace itself; it's formats still refer to the original transformation find though,
 // as transformations cannot be nested.
-formatWithIf = '{' n:integer ':+' ifContent:replace '}' { return { backreference: n, ifContent }; }
+formatWithIf = '{' n:integer ':+' ifContent:formatReplace '}' { return { backreference: n, ifContent }; }
 
 // Same as the if case, but resolve to `elseContent` if empty instead of the empty string
-formatWithIfElse = '{' n:integer ':?' ifContent:replace ':' elseContent:replace '}' { return { backreference: n, ifContent, elseContent }; }
+formatWithIfElse = '{' n:integer ':?' ifContent:ifElseReplace ':' elseContent:formatReplace '}' { return { backreference: n, ifContent, elseContent }; }
 
 // Same as the if case, but reversed behaviour with empty vs non-empty `n`th match
-formatWithElse = '{' n:integer ':' '-'? elseContent:replace { return { backreference: n, elseContent }; }
+// NOTE: The ':' form can cause ambiguities when the contents starts with '/', '+', etc.
+//       However, instead of allowing them to be escaped, just tell issue raisers to use the
+//       less ambiguous ':-' form (which also has nice symmetry with the ':+' form).
+formatWithElse = '{' n:integer ':' '-'? elseContent:formatReplace '}' { return { backreference: n, elseContent }; }
 
 // Used in `format`s to transform a string using a JS function
 modifier = '/' modifier:name { return modifier; }
@@ -153,14 +165,14 @@ variableWithTransform = '{' v:name t:transformation '}' { return { variable: v,
 // Top level text. Anything that cannot be the start of something special. False negatives are handled later by the `any` rule
 text = t:([^$\\}])+ { return t.join("") }
 
-// None-special text inside a tab stop placeholder. Should be no different to regular top level text.
+// Non-special text inside a tab stop placeholder. Should be no different to regular top level text.
 tabStopText = text
 
-// None-special text inside a choice. $, {, }, etc. are all regular text in this context.
+// Non-special text inside a choice. $, {, }, etc. are all regular text in this context.
 choiceText = b:(t:[^,|\\]+ { return t.join(""); } / '\\' c:[,|\\] { return c; } / '\\' c:. { return '\\' + c; } )+ { return b.join(""); }
 
-// None-special text inside a replace (substitution part of transformation). Same as normal text, but `/` is special (the end of the regex-like pattern)
-replaceText = t:[^$\\}/]+ { return t.join(""); }
+// Non-special text inside a replace (substitution part of transformation). Same as normal text, but `/` and ':' is special (the end of the regex-like pattern and if half terminator for if-else format)
+replaceText = t:[^$\\}/:]+ { return t.join(""); }
 
 // Match an escaped character. The set of characters that can be escaped is based on context, generally restricted to the minimum set that enables expressing any text content
 escapedTopLevel = '\\' c:[$\\}] { return c; }
@@ -174,8 +186,14 @@ escapedChoice = '\\' c:[$\\,|] { return c; }
 // Same as top level, but `/` can also be escaped
 escapedReplace = '\\' c:[$\\/] { return c; }
 
+// Format terminated by '}' instead of '/'
+escapedFormatReplace = '\\' c:[$\\}] { return c; }
+
+// If half of if-else format terminated by ':' instead of '}'
+escapedIfElseReplace = '\\' c:[$\\:] { return c; }
+
 // We handle 'modifiers' separately to escapes. These indicate a change in state when building the replacement (e.g., capitalisation)
-replaceModifier = '\\' m:[ElLuUnr] { return { modifier: m }; }
+replaceModifier = '\\' m:[ElLuU] { return { modifier: m }; }
 
 // Match nonnegative integers like those used for tab stop ordering
 integer = digits:[0-9]+ { return parseInt(digits.join(""), 10); }
diff --git a/spec/body-parser-spec.js b/spec/body-parser-spec.js
@@ -1,6 +1,9 @@
 const SnippetParser = require('../lib/snippet-body-parser');
 
 describe("Snippet Body Parser", () => {
+  // Helper for testing a snippet parse tree. The `input`
+  // is the snippet string, the `tree` is the expected
+  // parse tree.
   function expectMatch(input, tree) {
     expect(SnippetParser.parse(input)).toEqual(tree);
   }
@@ -30,6 +33,10 @@ describe("Snippet Body Parser", () => {
     });
 
     describe("with placeholders", () => {
+      it("allows placeholders to be empty", () => {
+        expectMatch("${1:}", [{ index: 1, content: [] }]);
+      });
+
       it("allows placeholders to be arbitrary", () => {
         expectMatch("${1:${2}$foo${3|a,b|}}", [
           {
@@ -43,6 +50,37 @@ describe("Snippet Body Parser", () => {
         ]);
       });
 
+      it("even lets placeholders contain placeholders", () => {
+        expectMatch("${1:${2:${3:levels}}}", [
+          {
+            index: 1,
+            content: [
+              {
+                index: 2,
+                content: [
+                  {
+                    index: 3,
+                    content: [
+                      "levels"
+                    ]
+                  }
+                ]
+              }
+            ]
+          }
+        ]);
+      });
+
+      it("ends the placeholder at an unmatched '}'", () => {
+        expectMatch("${1:}}", [
+          {
+            index: 1,
+            content: []
+          },
+          "}"
+        ]);
+      });
+
       it("allows escaping '}' in placeholders", () => {
         expectMatch("${1:\\}}", [{ index: 1, content: ["}"] }]);
       });
@@ -110,6 +148,8 @@ describe("Snippet Body Parser", () => {
       });
     });
 
+    // The placeholder implementation is expected to be the same as for tab stops, so
+    // see the tab stop placeholder section for more thorough tests
     describe("with placeholders", () => {
       it("allows placeholders to be arbitrary", () => {
         expectMatch("${foo:${2}$bar${3|a,b|}}", [
@@ -208,6 +248,153 @@ describe("Snippet Body Parser", () => {
         "/}"
       ]);
     });
+
+    it("allows and preserves all escapes in regex strings", () => {
+      expectMatch("${1/foo\\/\\$\\:\\n\\r/baz/}", [
+        {
+          index: 1,
+          transformation: {
+            find: /foo\/\$\:\n\r/,
+            replace: [
+              "baz"
+            ]
+          }
+        }
+      ]);
+    });
+
+    describe("When parsing the replace section", () => {
+      // Helper for testing the relacement part of
+      // transformations, which are relatively deep in
+      // the tree and have a lot of behaviour to cover
+      // NOTE: Only use when the replace section is expected to
+      // be valid, or else you will be testing against the
+      // boilerplate (which is not a good idea)
+      function expectReplaceMatch(replace, tree) {
+        expectMatch(`\${1/foo/${replace}/}`, [
+          {
+            index: 1,
+            transformation: {
+              find: /foo/,
+              replace: tree,
+            }
+          }
+        ]);
+      }
+
+      it("allows '$' and '}' as plain text if not part of a format", () => {
+        expectReplaceMatch("$}", ["$}"]);
+      });
+
+      it("allows inline 'escaped modifiers'", () => {
+        expectReplaceMatch("foo\\E\\l\\L\\u\\Ubar", [
+          "foo",
+          { modifier: "E" },
+          { modifier: "l" },
+          { modifier: "L" },
+          { modifier: "u" },
+          { modifier: "U" },
+          "bar"
+        ]);
+      });
+
+      it("allows '$', '\\', and '/' to be escaped", () => {
+        expectReplaceMatch("\\$1 \\\\ \\/", [
+          "$1 \\ /"
+        ]);
+      });
+
+      describe("When parsing formats", () => {
+        it("parses simple formats", () => {
+          expectReplaceMatch("$1${2}", [
+            { backreference: 1 },
+            { backreference: 2 }
+          ]);
+        });
+
+        it("parses formats with modifiers", () => {
+          expectReplaceMatch("${1:/upcase}", [
+            {
+              backreference: 1,
+              modifier: "upcase",
+            }
+          ]);
+        });
+
+        it("parses formats with an if branch", () => {
+          expectReplaceMatch("${1:+foo$2$bar}", [
+            {
+              backreference: 1,
+              ifContent: [
+                "foo",
+                { backreference: 2, },
+                "$bar" // no variables inside a replace / format
+              ]
+            }
+          ]);
+        });
+
+        it("parses formats with if and else branches", () => {
+          expectReplaceMatch("${1:?foo\\:stillIf:bar\\}stillElse}", [
+            {
+              backreference: 1,
+              ifContent: [
+                "foo:stillIf"
+              ],
+              elseContent: [
+                "bar}stillElse"
+              ]
+            }
+          ]);
+        });
+
+        it("parses formats with an else branch", () => {
+          expectReplaceMatch("${1:-foo}", [
+            {
+              backreference: 1,
+              elseContent: [
+                "foo"
+              ]
+            }
+          ]);
+        });
+
+        it("parses formats with the old else branch syntax", () => {
+          expectReplaceMatch("${1:foo}", [
+            {
+              backreference: 1,
+              elseContent: [
+                "foo"
+              ]
+            }
+          ]);
+        });
+
+        it("allows nested replacements inside of formats", () => {
+          expectReplaceMatch("${1:+${2:-${3:?a lot of:layers}}}", [
+            {
+              backreference: 1,
+              ifContent: [
+                {
+                  backreference: 2,
+                  elseContent: [
+                    {
+                      backreference: 3,
+                      ifContent: [
+                        "a lot of"
+                      ],
+                      elseContent: [
+                        "layers"
+                      ]
+                    }
+                  ]
+                }
+              ]
+            }
+          ]);
+        });
+      });
+    });
   });
 
   describe("When parsing escaped characters", () => {
