Refactor Parser #1723
Refactor Parser #1723
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1723 +/- ##
==========================================
+ Coverage 93.95% 93.99% +0.03%
==========================================
Files 78 78
Lines 4584 4545 -39
Branches 809 792 -17
==========================================
- Hits 4307 4272 -35
+ Misses 245 242 -3
+ Partials 32 31 -1
Continue to review full report at Codecov.
|
|
I'm excited about the removal of |
| } | ||
| rowGaps.push(assertNodeType(cr, "cr").size); | ||
| const cr = assertNodeType(parser.parseFunction(), "cr"); | ||
| rowGaps.push(cr.size); |
There was a problem hiding this comment.
I guess this simpler error checking is fine because parseFunction will return null only if \cr is not a defined function, which shouldn't happen. Ditto below for \right.
There was a problem hiding this comment.
assertNodeType will throw if parser.parseFunction() returns null.
|
@edemaine Thank you for the review! |
There was a problem hiding this comment.
@ylemkimon thank you for this awesome PR, sorry for not looking at it in detail sooner. @edemaine thanks for reviewing this!
| optional?: boolean, | ||
| greediness?: ?number, | ||
| breakOnTokenText?: BreakToken, | ||
| mode?: Mode, |
There was a problem hiding this comment.
Now that we have so many optional params here we might want to replace them with an a single options object param.
| if (!this.settings.throwOnError && lex.text[0] === "\\") { | ||
| const errorNode = this.handleUnsupportedCmd(); | ||
| body.push(errorNode); | ||
| continue; |
There was a problem hiding this comment.
Why is this no longer necessary?
There was a problem hiding this comment.
I see, it looks like it got moved inside parseGroup.
| "Can't use function '" + func + "' in text mode", token); | ||
| } else if (this.mode === "math" && funcData.allowedInMath === false) { | ||
| throw new ParseError( | ||
| "Can't use function '" + func + "' in math mode", token); |
There was a problem hiding this comment.
This is really nice. I like how you collected a number of different errors in the same place.
| // function and say that it is a function. | ||
| // The token will be consumed later in parseGivenFunction | ||
| // (after possibly switching modes). | ||
| return newFunction(nucleus); |
There was a problem hiding this comment.
Getting rid of this sure allowed you clean up a lot of things.
| } | ||
| rowGaps.push(assertNodeType(cr, "cr").size); | ||
| const cr = assertNodeType(parser.parseFunction(), "cr"); | ||
| rowGaps.push(cr.size); |
There was a problem hiding this comment.
assertNodeType will throw if parser.parseFunction() returns null.
| } | ||
|
|
||
| if (funcName === "\\begin") { | ||
| // begin...end is similar to left...right |
There was a problem hiding this comment.
This is easier to follow. I'm glad that you were able to make this look a little more like delimsizing.js.
parseGroupparseGivenFunctionto parse only function, removeparseFunction, and renameparseGivenFunctiontoparseFunction. Moved argument greediness check toparseFunctionparseImplicitGroupandparseGroupParsedFuncandParsedArgparseFunctioncall fromparseSymboltoparseGroupThis is a step toward #428.