Port environments.js to @flow. #862

marcianx · 2017-09-09T22:00:57Z

Fixes #859.
Sits on top of PR 860, which needs to be submitted first!

kevinbarabash · 2017-09-09T22:11:27Z

src/environments.js

+  envName: string,
+  parser: Parser,
+  positions: number[],
+};


nit: 4 space tab.

Oi! Time to change my vim defaults. I keep forgetting to set it to 4 spaces in every new session. ;)

kevinbarabash · 2017-09-09T22:12:43Z

src/environments.js

+ * List of ParseNodes followed by an array of positions.
+ * Returned by `Parser`'s `parseArguments`.
+ */
+type ParseResult = (ParseNode | number[])[] | ParseNode;


When do we return a result with type number[][]?

Parser's parseArgument always returns the list of positions as the last element.
So it returns something of the format [ParseNode, ParseNode, ..., ParseNode, number[]]. So this happens any time you see a defineFunction with code like

res = parseArray(context.parser, res, "display"); return res;

We need to refactor this at some point, but I'd rather do it right before or during ParseNode being ported. It would help to have maximal support from type-safety while doing this since ParseResult is simply stored here and used various places (including possible buildHtml, but I haven't read that part of the code base yet).

I see, it's documented in the comment above.

We need to refactor this at some point, but I'd rather do it right before or during ParseNode being ported.

Sounds good.

kevinbarabash · 2017-09-09T22:14:25Z

src/environments.js

+    allowedInText: boolean,
+    numOptionalArgs: number,
+    handler: EnvHandler,
+};


This looks suspiciously similar to how functions are typed. We might be able to dedupe in future.

FYI: Looking at defineFunction.js I don't currently see any benefit in coalescing.

We don't really need any subtype relationship.

For object/struct types, there's not really an extends type behavior AFAICS (please correct me if I'm wrong), so we would need to repeat the fields anyway when defining it.

kevinbarabash · 2017-09-09T22:18:03Z

src/environments.js

+ *  - numArgs: (default 0) The number of arguments after the \begin{name} function.
+ *  - argTypes: (optional) Just like for a function
+ *  - allowedInText: (default false) Whether or not the environment is allowed
+ *  i                nside text mode (not enforced yet).


i nside => inside

kevinbarabash · 2017-09-09T22:22:13Z

src/environments.js

+    pregap?: number,
+    postgap?: number,
+};
+type EnvNodeData = {


ArrayEnvNode or ArrayNode would be better as this type is specific to arrays. For consistency ArrayColSpec could be AlignNode and we could even introduce a SeparateNode type.

ArrayEnvNode is more descriptive. So I'm using that.
Is AlignNode the most appropriate name? It's not a ParseNode.

Also, per ArrayEnvNode, it's not a ParseNode, but the data stored in the ParseNode. Would you still prefer to call it ArrayEnvNode? Or would ArrayEnvNodeData (a little long, I know) be more appropriate to avoid confusion?

In the currently-pushed version, I went with AlignSpec and ArrayEnvNodeData. I'm very open to changing per your feedback.

I don't see a need to differentiate parse nodes from environment nodes as they end up being part of the same tree at the end of the day. I think eventually we'll types for each of the different nodes the parser can generate and eventually ParseNode will become:

ParseNode = FracNode | ExpNode | ... | ArrayNode | AlignNode | ...

Environments are just one type of thing that the parser parses as it does its work, it just happens that the environment parsing code is in a separate file.

I'm sorry, I didn't quite understand this comment and the future evolution you intend. What I meant was: for result: ArrayEnvNodeData, we have

return new ParseNode(result.type, result, parser.mode);

So the ParseNode instance contains a payload that is of type ArrayEnvNodeData struct, which is why I was thinking about the Data suffix.

So did you mean that the payload would become a union type (e.g. ParseNodeData or something briefer)?
I'm guessing you didn't mean that the ParseNode class would be dissolved in favor of a union type (given that we still want to encapsulate the common tokenizer metadata for error reporting)?

I was thinking that eventually we'd have a union type to specify the different types in the tree. It's possible that ParseNode wraps these data nodes and adds additional data like error reporting. I have spent any time thinking trough the details. The high-level goal is for nodes with in the parse tree to be more regular and for all them to be statically typed.

Ok, so what you're saying is that the payload of ParseNode is still considered an undecorated "Node". And hence we should use the Node suffix rather than NodeData?
Is that your preference?

Ah, I see that you merged it. Please LMK what nomenclature you had in mind. I'm happy to update these as we continue in the future.

kevinbarabash · 2017-09-09T22:22:52Z

src/environments.js

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


Why the change?

Ah, that was just to get rid of the eslint suppression at the top (personally a fan of maximal linting). However, that's not my favorite lint anyway; and I can revert this if you like.

Gotcha. This is fine. We can re-evaluate lint rules separately.

kevinbarabash · 2017-09-09T22:22:58Z

src/environments.js

+    parser: Parser,
+    result: EnvNodeData,
+    style: StyleStr,
+) {


kevinbarabash · 2017-09-09T22:24:39Z

src/environments.js

 */
-function defineEnvironment(names, props, handler) {
+function defineEnvironment(
+    names: string | string[],


We updated defineFucntion to only take an array of strings. We should probably do the same here.

kevinbarabash · 2017-09-09T22:25:40Z

src/environments.js

-function defineEnvironment(names, props, handler) {
+function defineEnvironment(
+    names: string | string[],
+    props: EnvProps | number,


defineFunction doesn't support taking a number for props, we should do the same here too.

kevinbarabash · 2017-09-10T00:31:36Z

@marcianx thanks for the PR. It's exciting to see all of these implicit types being made explicit.

* Port environment.js to @flow. * Updated per comments.

marcianx changed the title ~~Flow environment~~ Port environment.js to @flow. Sep 9, 2017

marcianx changed the title ~~Port environment.js to @flow.~~ Port environments.js to @flow. Sep 9, 2017

marcianx force-pushed the flow-environment branch from 990747d to a5bf8f5 Compare September 9, 2017 22:03

Port environment.js to @flow.

431fc07

marcianx force-pushed the flow-environment branch from a5bf8f5 to 431fc07 Compare September 9, 2017 22:13

kevinbarabash reviewed Sep 9, 2017

View reviewed changes

Updated per comments.

a7063ee

kevinbarabash approved these changes Sep 10, 2017

View reviewed changes

kevinbarabash merged commit 1c344ed into KaTeX:master Sep 10, 2017

marcianx deleted the flow-environment branch September 10, 2017 00:32

marcianx added a commit to marcianx/KaTeX that referenced this pull request Sep 10, 2017

Port environments.js to @flow. (KaTeX#862)

abb9825

* Port environment.js to @flow. * Updated per comments.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port environments.js to @flow. #862

Port environments.js to @flow. #862

marcianx commented Sep 9, 2017

kevinbarabash Sep 9, 2017

marcianx Sep 9, 2017

kevinbarabash Sep 9, 2017

marcianx Sep 9, 2017 •

edited

kevinbarabash Sep 9, 2017

kevinbarabash Sep 9, 2017

marcianx Sep 9, 2017

marcianx Sep 10, 2017

kevinbarabash Sep 9, 2017

marcianx Sep 9, 2017

kevinbarabash Sep 9, 2017

marcianx Sep 9, 2017

marcianx Sep 9, 2017

marcianx Sep 9, 2017

kevinbarabash Sep 9, 2017

marcianx Sep 10, 2017

kevinbarabash Sep 10, 2017

marcianx Sep 10, 2017

marcianx Sep 10, 2017

kevinbarabash Sep 9, 2017

marcianx Sep 9, 2017

kevinbarabash Sep 9, 2017

kevinbarabash Sep 9, 2017

kevinbarabash Sep 9, 2017

marcianx Sep 9, 2017

kevinbarabash Sep 9, 2017

marcianx Sep 9, 2017

kevinbarabash commented Sep 10, 2017

Port environments.js to @flow. #862

Port environments.js to @flow. #862

Conversation

marcianx commented Sep 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcianx Sep 9, 2017 • edited

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

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

kevinbarabash commented Sep 10, 2017

marcianx Sep 9, 2017 •

edited