allow sizing commands inside optional groups#885

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

Fixes #650 and #516.

TODO:

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

src/Parser.js

            }
-            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.

k4b7 · 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 [ ... ], }

k4b7 · 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

                break;
            }
-            const atom = this.parseAtom();
+            // const optional = breakOnTokenText === "]";


Should this now be removed?

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

src/Parser.js

     * @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

     * @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]).

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

src/Parser.js

            // 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

            // 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

k4b7 · 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

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

k4b7 commented Sep 16, 2017

View reviewed changes

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

k4b7 mentioned this pull request Sep 19, 2017

Port Parser.js to @flow #892

Closed

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

marcianx reviewed Sep 26, 2017

View reviewed changes

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

k4b7 commented Sep 27, 2017

View reviewed changes

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

allow sizing commands inside optional groups

7bfc125

k4b7 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

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

k4b7 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

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

k4b7 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

k4b7 merged commit 71e0b35 into master Oct 3, 2017

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

Uh oh!

Comments

Conversation

k4b7 commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Sep 26, 2017

Uh oh!

marcianx commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcianx left a comment

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

k4b7 commented Sep 15, 2017 •

edited

Loading

k4b7 commented Oct 3, 2017 •

edited

Loading