allow sizing commands inside optional groups #885

kevinbarabash · 2017-09-15T05:35:36Z

Fixes #650 and #516.

TODO:

edemaine · 2017-09-15T13:06:49Z

src/Parser.js

@@ -165,7 +165,8 @@ class Parser {
            if (breakOnInfix && functions[lex.text] && functions[lex.text].infix) {
                break;
            }
-            const atom = this.parseAtom();
+            const optional = breakOnTokenText === "]";
+            const atom = this.parseAtom(optional);


I haven't thought about this deeply, but is there a reason to just pass in ] and not just breakOnTokenText in general? I think the latter might be useful for more general macro support in the future... Maybe worth reviewing what other values this currently takes on in the code, if any.

Good idea. I'll look into this evening.

kevinbarabash · 2017-09-16T05:46:53Z

test/__snapshots__/katex-spec.js.snap

+            "type": "sizing",
+            "value": Object {
+              "size": 5,
+              "value": Array [


This is kind of weird. I hope that in the future that sizing nodes could maybe look like:

ParseNode: { mode: "math", type: "sizing", size: 5, body: Array [ ... ], }

kevinbarabash · 2017-09-26T03:57:14Z

I'm going to update the jsDocs to include the new breakOnTokenText. @marcianx would you be able to review this PR?

marcianx · 2017-09-26T04:03:27Z

Will do. I'll do one pass now. And if there are any changes. Then one in the morning.

marcianx · 2017-09-26T04:05:16Z

src/Parser.js

@@ -165,7 +165,8 @@ class Parser {
            if (breakOnInfix && functions[lex.text] && functions[lex.text].infix) {
                break;
            }
-            const atom = this.parseAtom();
+            // const optional = breakOnTokenText === "]";


Should this now be removed?

marcianx · 2017-09-26T04:14:10Z

src/Parser.js

@@ -308,10 +309,10 @@ class Parser {
     *
     * @return {?ParseNode}
     */
-    parseAtom() {
+    parseAtom(breakOnTokenText) {


nit: Mind adding a @param documenting the type of the new parameter?

marcianx · 2017-09-26T04:14:22Z

src/Parser.js

@@ -425,7 +426,7 @@ class Parser {
     *
     * @return {?ParseNode}
     */
-    parseImplicitGroup() {
+    parseImplicitGroup(breakOnTokenText) {


Same here, please.

marcianx · 2017-09-26T04:22:43Z

test/__snapshots__/katex-spec.js.snap

+          },
+        ],
+      },
+      "type": "sqrt",


(optional) I don't know whether an arbitrary strict order is enforced here, but if not, moving this up would make it much easier to see which level it is in.

I can set up a custom formatter for the snapshots to always put "type" first.

marcianx · 2017-09-26T04:37:00Z

src/Parser.js

        // The body of an atom is an implicit group, so that things like
        // \left(x\right)^2 work correctly.
-        const base = this.parseImplicitGroup();
+        const base = this.parseImplicitGroup(breakOnTokenText);


It might be worth adding a comment somewhere as to why it's okay not to pass this parameter down to other methods below. The reason seems to be subtle: as handleSupSubscript can call parseGroup which can call parseExpression, but parseExpression is only called in parseGroup only if a { or [ is encountered, opening a deeper layer. But I haven't yet done an exhaustive check.

I'm also fine with completely deferring this if we intend to formalize the various non-obvious invariants in this file some point to aid comprehensibility so that they aren't violated in the future.

I'll add a comment. handleSupSubscript should always be parsing non-optional groups afaict because x^[2] isn't valid (or rather it's valid it's treated as x^{[}2]).

kevinbarabash · 2017-09-27T15:48:01Z

src/Parser.js

@@ -834,7 +836,7 @@ export default class Parser {
        if (this.nextToken.text === (optional ? "[" : "{")) {
            // If we get a brace, parse an expression
            this.consume();
-            const expression = this.parseExpression(false, optional ? "]" : null);
+            const expression = this.parseExpression(false, optional ? "]" : "}");


I'm not sure why this was null.

Parsing always ends on }, so I'm not sure specifying "}" is necessary... hence the null. (It's been a while since I've looked at this code, so I forget the exact details...)

It feels nice to have symmetry the ? "[" : "{" check above.

marcianx · 2017-09-28T01:36:42Z

src/Parser.js

@@ -527,7 +529,7 @@ export default class Parser {
        } else if (utils.contains(Parser.sizeFuncs, func)) {
            // If we see a sizing function, parse out the implicit body
            this.consumeSpaces();
-            const body = this.parseExpression(false);
+            const body = this.parseExpression(false, breakOnTokenText);


Why does this only apply to this block and not to the rest below: styleFuncs, oldFontFuncs, and \color? Are they not similarly affected? Or do none of our functions currently want to allow these in optional groups?
Otherwise, this PR mostly LGTM.

Good question. I was fixing only the issue, but I'm sure there are places where KA also uses \color inside of optional groups (apparently LaTeX supports this too). LaTeX also supports style functions within optional blocks too.

marcianx

LGTM

kevinbarabash · 2017-10-03T17:31:27Z

@marcianx thanks for the review. Feel free to merge PRs as well.

edemaine reviewed Sep 15, 2017

View reviewed changes

kevinbarabash force-pushed the sizing_in_option_groups branch from 1bfae05 to 80e4b68 Compare September 16, 2017 05:42

kevinbarabash commented Sep 16, 2017

View reviewed changes

kevinbarabash added the GH Review: review-needed label Sep 16, 2017

kevinbarabash mentioned this pull request Sep 19, 2017

Port Parser.js to @flow #892

Closed

kevinbarabash added GH Review: needs-revision and removed GH Review: review-needed labels Sep 26, 2017

marcianx reviewed Sep 26, 2017

View reviewed changes

kevinbarabash force-pushed the sizing_in_option_groups branch from 80e4b68 to b44efb3 Compare September 27, 2017 15:46

kevinbarabash commented Sep 27, 2017

View reviewed changes

kevinbarabash added GH Review: review-needed and removed GH Review: needs-revision labels Sep 27, 2017

allow sizing commands inside optional groups

7bfc125

kevinbarabash force-pushed the sizing_in_option_groups branch from 29827f5 to 7bfc125 Compare September 27, 2017 16:00

marcianx reviewed Sep 28, 2017

View reviewed changes

kevinbarabash added GH Review: needs-revision and removed GH Review: review-needed labels Sep 29, 2017

kevinbarabash force-pushed the sizing_in_option_groups branch from 91ac174 to 5ba86ca Compare September 30, 2017 04:07

allow color, old font, and style commands inside optional groups

326f17f

kevinbarabash force-pushed the sizing_in_option_groups branch from 5ba86ca to 326f17f Compare September 30, 2017 04:08

kevinbarabash added GH Review: review-needed and removed GH Review: needs-revision labels Sep 30, 2017

marcianx approved these changes Oct 1, 2017

View reviewed changes

marcianx added GH review: accepted and removed GH Review: review-needed labels Oct 3, 2017

Merge branch 'master' into sizing_in_option_groups

be8c0c9

kevinbarabash merged commit 71e0b35 into master Oct 3, 2017

kevinbarabash deleted the sizing_in_option_groups branch November 27, 2017 01:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow sizing commands inside optional groups #885

allow sizing commands inside optional groups #885

kevinbarabash commented Sep 15, 2017 •

edited

Loading

edemaine Sep 15, 2017

kevinbarabash Sep 15, 2017

kevinbarabash Sep 16, 2017

kevinbarabash commented Sep 26, 2017

marcianx commented Sep 26, 2017

marcianx Sep 26, 2017

marcianx Sep 26, 2017

marcianx Sep 26, 2017

marcianx Sep 26, 2017

kevinbarabash Sep 26, 2017

marcianx Sep 26, 2017

kevinbarabash Sep 27, 2017

kevinbarabash Sep 27, 2017

edemaine Sep 27, 2017

kevinbarabash Sep 27, 2017

marcianx Sep 28, 2017

kevinbarabash Sep 29, 2017

marcianx left a comment

kevinbarabash commented Oct 3, 2017 •

edited

Loading

allow sizing commands inside optional groups #885

allow sizing commands inside optional groups #885

Conversation

kevinbarabash commented Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash commented Sep 26, 2017

marcianx commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcianx left a comment

Choose a reason for hiding this comment

kevinbarabash commented Oct 3, 2017 • edited Loading

kevinbarabash commented Sep 15, 2017 •

edited

Loading

kevinbarabash commented Oct 3, 2017 •

edited

Loading