Changed alias expansion to allow for nested aliases #125
Conversation
| return char; | ||
| } | ||
| else if (i >= query.length) { | ||
| throw new Error( |
There was a problem hiding this comment.
Consider throwing your own class here that inherits from Error. Then when you catch, use instanceof operator to detect your error. Re-throw other errors.
| //Intrinsics | ||
| const whitespace = () => match(' '); | ||
| const letter = () => | ||
| match( |
There was a problem hiding this comment.
Consider using on of the built-in iswhitespace methods here to handle other locals.
There was a problem hiding this comment.
If not, then define the constant outside of the function. (To avoid performing string concat every call)
There was a problem hiding this comment.
Overall, looks good. Could shuffle around the organization for better readability.
Would be best to add a description of the overall algorithm being used here. Documentation is useful to bootstrap one's mind into a context where it's easier to grok what they're reading. Reverse engineering functional code is fun, but can also be time consuming if coming from a cold start.
I've found that not everyone is familiar with higher mathematics, so it may be good to define what functions like kleene and kleeneStar do. Not everyone is familiar with the formal notation from which regular expressions are derived (or they may need a refresher).
Also, I would add some tests showing deeper recursion, if I was going to use this in a prod env.
| //Intrinsics | ||
| const whitespace = () => match(' '); | ||
| const letter = () => | ||
| match( |
There was a problem hiding this comment.
If not, then define the constant outside of the function. (To avoid performing string concat every call)
| if (c.includes(char)) { | ||
| i++; | ||
| return char; | ||
| } |
There was a problem hiding this comment.
} should be on the same line as else if
Run gts check at project root to run linter to see style issues in the project as a whole.
|
|
||
| yield* aliasesFromPatternHelper(results); | ||
|
|
||
| function kleene<T>(match: () => T, separator?: () => void): T[] { |
There was a problem hiding this comment.
Overloading the term match. Within this namespace, the word match means very different things within different lexical scopes.
Please avoid instances of variable overshadowing here, and in other locations (such as i in for loops, since i is defined in the outer scope)
| separator(); | ||
| kleene(whitespace); | ||
| }; | ||
| const group = () => kleenePlus(phrase, listGap); |
There was a problem hiding this comment.
May be good, for the sake of readability to define phrase before introducing group
| } | ||
|
|
||
| const term = () => union(choice, optional, token); | ||
| const phrase = () => cartesianProduct(kleenePlus(term)).map(a => a.join('')); |
There was a problem hiding this comment.
Haha, took a second to decompose this one.
kleenePlus(...) generates a list of branches (where each branch is a list of tokens), then the Cartesian enumerates all paths through these branches.
Wouldn't be a bad idea to document this! A little explanation by the dev saves a lot of reverse engineering by the reader.
Added support for expanding nested aliases and updated tests.