Skip to content

Parser: Separate mandatory and optional arguments in parseArguments#903

Merged
k4b7 merged 2 commits intoKaTeX:masterfrom
marcianx:parse-args
Sep 26, 2017
Merged

Parser: Separate mandatory and optional arguments in parseArguments#903
k4b7 merged 2 commits intoKaTeX:masterfrom
marcianx:parse-args

Conversation

@marcianx
Copy link
Copy Markdown
Collaborator

This is for improved type strictness per one of the TODOs toward #892.

This overlaps in a minor way with PR 901, but since I don't know whether the former would require any discussion, I kept this PR independent.
It does require manual merging/rebasing (automatic doesn't work), but it's trivial.

@marcianx marcianx requested a review from k4b7 September 23, 2017 21:29
* @param {string} name
* @param {Array<ParseNode>} args
* @param {Array<?ParseNode>} optArgs
* @param {Token=} token
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the plan to turn this into flow types at a later date?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The point is to write down all our intended types while we refactor to make porting to flow possible. That requires the ParseNode refactor we discussed in #892. Until we're there, writing out these types aids keeping our intended mental model better documented.

const {args, optArgs} = this.parseArguments(func, funcData);
const token = baseGroup.token;
const result = this.callFunction(func, args, token);
const result = this.callFunction(func, args, optArgs, token);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@k4b7
Copy link
Copy Markdown
Member

k4b7 commented Sep 24, 2017

Looks like something changed in the output of the Accents screenshot.

@marcianx
Copy link
Copy Markdown
Collaborator Author

marcianx commented Sep 25, 2017

Regarding screenshots, i'm having trouble getting them working. At least the screenshot server starts in master, but in this branch, I run into

Accents
[...]/KaTeX/node_modules/selenium-webdriver/lib/promise.js:654
    throw error;
    ^

WebDriverError: javascript error: handle_search_string is not defined
JavaScript stack:
ReferenceError: handle_search_string is not defined
    at eval (eval at executeAsyncScript (unknown source), <anonymous>:2:68)
    at eval (eval at executeAsyncScript (unknown source),
 <anonymous>:2:226)
    at executeAsyncScript (<anonymous>:321:26)
    at <anonymous>:337:29
    at callFunction (<anonymous>:229:33)
    at <anonymous>:239:23
    at <anonymous>:240:3
    at Object.InjectedScript._evaluateOn (<anonymous>:875:140)
    at Object.InjectedScript._evaluateAndWrap (<anonymous>:808:34)
    at Object.InjectedScript.evaluate (<anonymous>:664:21)

Seems to suggest something being wrong with callFunction perhaps.

It did seem to work on subsequent attempts, but the Chrome screenshot said web page not found and the Firefox screenshot said simply "Test".

Running the Accent LaTeX \vec{A}\vec{x}\vec x^2\vec{x}_2^2\vec{A}^2\vec{xA}^2 on a local html shows no errors whatsoever (see attached png). Any clues?
accent-test-local

@k4b7
Copy link
Copy Markdown
Member

k4b7 commented Sep 25, 2017

I'm not sure what's going on with handle_search_string locally. I'm not seeing that in travis-ci report. The reason why there might be an issue with the rendering is that screenshot is too big:

Error: Expected 1024 x 768, got 1024x839

@marcianx
Copy link
Copy Markdown
Collaborator Author

Must have been something flaky. After rebasing to new master, all checks seem to succeed on Travis.

Copy link
Copy Markdown
Member

@k4b7 k4b7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

let arg = argType ?
this.parseGroupOfType(argType, isOptional) :
this.parseGroup(isOptional);
if (!arg) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice cleanup

@k4b7 k4b7 merged commit 6de5446 into KaTeX:master Sep 26, 2017
@k4b7
Copy link
Copy Markdown
Member

k4b7 commented Sep 26, 2017

@marcianx thanks for the PR and continuing to make progress on #892.

@marcianx marcianx deleted the parse-args branch September 26, 2017 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants