refactor defineEnvironment to also accept HTML and MathML builders #875

kevinbarabash · 2017-09-12T00:06:36Z

fixes #870

kevinbarabash · 2017-09-12T03:46:59Z

src/environments.js

+    },
+    get(envName: string) {
+        return _environments[envName];
+    },


@marcianx here's what I did for the wrapper around _environments.

kevinbarabash · 2017-09-12T03:48:04Z

src/defineEnvironment.js

+ * 2. requires all arguments except argType
+ * It is generated by `defineEnvironment()` below.
+ */
+type EnvSpec = {|


I tightened up some of these types by updating them to use exact object types.

Meta-comment on GitHub reviews vs. Phabricator: I'm finding it frustrating that comments need to pin to a single line.

kevinbarabash · 2017-09-12T03:53:35Z

I thought we'd end up with separate files for \*matrix, \*cases, etc., but it turns out they all use the same htmlBuilder and mathmlBuilder so for now they're all in the same file. I still think this is the right direction because it groups all the related code in the same file and it also opens the door to eventually expose defineEnvironment at some point which wasn't possible the way things were structured before.

…ixes #870

kevinbarabash · 2017-09-19T04:11:57Z

src/buildHTML.js

@@ -581,166 +581,6 @@ groupTypes.genfrac = function(group, options) {
        options);
 };

-groupTypes.array = function(group, options) {


Copied to const htmlBuilder = function(group, options) { ... in array.js.

Thanks for leaving hints like this!

kevinbarabash · 2017-09-19T04:15:31Z

src/environments/array.js

+// is part of the source2e.pdf file of LaTeX2e source documentation.
+// {darray} is an {array} environment where cells are set in \displaystyle,
+// as defined in nccmath.sty.
+defineEnvironment({


The calls to defineEnvironment look a little different from how they looked in environments.js. All of the existing parameters have been converted props in a single object argument. This was done so it's easier to know which param is which.

kevinbarabash · 2017-09-19T04:15:42Z

src/environments/array.js

+        return res;
+    },
+    htmlBuilder,
+    mathmlBuilder,


These are the two "new" parameters.

kevinbarabash · 2017-09-19T04:17:28Z

src/environments.js

@@ -1,326 +1,16 @@
 // @flow


Everything except for defineEnvironment and the type definitions have been copied over to array.js while the remaining code has gone to defineEnvironment.js.

kevinbarabash · 2017-09-19T04:19:20Z

src/defineEnvironment.js

+    }
+    if (mathmlBuilder) {
+        mathmlGroupTypes[type] = mathmlBuilder;
+    }


The body of defineEnvironment has remained largely the same. The new code adds registers the htmlBuilder and mathmlBuilder args for the given type. This was previously being done in buildHTML.js and buildMathML.js respectively.

rileyjshaw

Awesome! This is a big commit for my first foray into KaTeX, but the refactor looks sound. Since I'm unfamiliar with the library, my approval should be taken with a grain of salt; please test this thoroughly before landing.

I left a few questions, but nothing blocking. The motivation described in #870 makes sense to me.

Thanks for doing this!

rileyjshaw · 2017-09-19T13:40:50Z

src/environments.js

-    postgap?: number,
+import {_environments} from "./defineEnvironment";
+
+const environments = {


Cool! Is the motivation behind this abstraction to limit access to the _environments object? Or do you expect for the bodies of has and get to eventually become more complicated?

If this API was added solely for clarity, I don't think has and get improve readability over hasOwnProperty and [].

The motivation was to limit access. The plan was to do the same for _functions and _symbols. Not sure how necessary it is so I'm going to let this simmer for a while.

rileyjshaw · 2017-09-19T13:43:32Z

src/buildHTML.js

@@ -581,166 +581,6 @@ groupTypes.genfrac = function(group, options) {
        options);
 };

-groupTypes.array = function(group, options) {


Thanks for leaving hints like this!

rileyjshaw · 2017-09-19T13:50:40Z

src/environments/array.js

+    let row = [];
+    const body = [row];
+    const rowGaps = [];
+    for (;;) {


I know this is a copypasta, but while(true) would be clearer here.

It was originally while(true), but a previous commit changed it after removing a top level eslint-disable. I can add it back in with a eslint-disable-line.

rileyjshaw · 2017-09-19T13:53:23Z

src/defineEnvironment.js

+ * 2. requires all arguments except argType
+ * It is generated by `defineEnvironment()` below.
+ */
+type EnvSpec = {|


Meta-comment on GitHub reviews vs. Phabricator: I'm finding it frustrating that comments need to pin to a single line.

rileyjshaw · 2017-09-19T13:56:47Z

src/environments/array.js

+        "vmatrix",
+        "Vmatrix",
+    ],
+    props: {},


Should props be optional?

That's a good question. defineFunction requires numArgs on props even if it's 0. I'm not sure how useful that is, but I'd rather be consistent so I'll make it mandatory here too.

rileyjshaw · 2017-09-19T13:57:53Z

src/defineEnvironment.js

+ *  - numOptionalArgs: (default 0) Just like for a function
+ */
+type EnvProps = {
+    numArgs?: number,


I only see numArgs used in this review, but I imagine the rest are used elsewhere / externally?

Good catch. We may want to add those at some point in the future, but if they're not being used no reason to include them now.

…(true)

kevinbarabash force-pushed the refactor_environments branch 3 times, most recently from ed0b28a to 0c1caa5 Compare September 12, 2017 03:46

kevinbarabash commented Sep 12, 2017

View reviewed changes

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

kevinbarabash assigned rileyjshaw Sep 15, 2017

kevinbarabash requested a review from rileyjshaw September 15, 2017 17:01

kevinbarabash unassigned rileyjshaw Sep 15, 2017

kevinbarabash mentioned this pull request Sep 19, 2017

Port Parser.js to @flow #892

Closed

refactor defineEnvironment to also accept HTML and MathML builders, f…

59906e8

…ixes #870

kevinbarabash force-pushed the refactor_environments branch from eacad1e to 59906e8 Compare September 19, 2017 03:58

kevinbarabash commented Sep 19, 2017

View reviewed changes

rileyjshaw approved these changes Sep 19, 2017

View reviewed changes

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

make argTypes mandatory, remove unused props from EnvProps, use while…

598cff3

…(true)

kevinbarabash force-pushed the refactor_environments branch from 21190fa to 598cff3 Compare September 20, 2017 03:12

Merge branch 'master' into refactor_environments

d45d92b

kevinbarabash removed the GH Review: needs-revision label Sep 20, 2017

kevinbarabash merged commit ca6ea4c into master Sep 20, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor defineEnvironment to also accept HTML and MathML builders #875

refactor defineEnvironment to also accept HTML and MathML builders #875

kevinbarabash commented Sep 12, 2017

kevinbarabash Sep 12, 2017

kevinbarabash Sep 12, 2017

rileyjshaw Sep 19, 2017

kevinbarabash commented Sep 12, 2017

kevinbarabash Sep 19, 2017

rileyjshaw Sep 19, 2017

kevinbarabash Sep 19, 2017

kevinbarabash Sep 19, 2017

kevinbarabash Sep 19, 2017

kevinbarabash Sep 19, 2017

rileyjshaw left a comment

rileyjshaw Sep 19, 2017

kevinbarabash Sep 19, 2017 •

edited

Loading

rileyjshaw Sep 19, 2017

rileyjshaw Sep 19, 2017

kevinbarabash Sep 19, 2017

rileyjshaw Sep 19, 2017

rileyjshaw Sep 19, 2017

kevinbarabash Sep 19, 2017 •

edited

Loading

rileyjshaw Sep 19, 2017

kevinbarabash Sep 19, 2017

refactor defineEnvironment to also accept HTML and MathML builders #875

refactor defineEnvironment to also accept HTML and MathML builders #875

Conversation

kevinbarabash commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash commented Sep 12, 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

rileyjshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash Sep 19, 2017 • edited Loading

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

kevinbarabash Sep 19, 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 Sep 19, 2017 •

edited

Loading

kevinbarabash Sep 19, 2017 •

edited

Loading