Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for cdot and times LaTeX commands #6

Closed
wants to merge 6 commits into from

Conversation

whatl3y
Copy link
Contributor

@whatl3y whatl3y commented Jul 12, 2020

Fixes the times problem mentioned in #1, but could probably as you mention add a thorough test suite for LaTeX commands in the future. For my use case I at least need support for cdot and times. All other iterations I've tried for my needs work otherwise though.

@arthanzel
Copy link
Owner

Why are \cdot and \times implemented like the \frac macro? They're infix operators. The simplest way to implement them would be to replace their tokens with a TIMES token in the lexer.

What's the use case here?

@whatl3y
Copy link
Contributor Author

whatl3y commented Jul 27, 2020

Apologies, I read through the code to get a better understanding and made an incorrect assumption I should use the localFunctions pattern to add support for these instead of the lexer (since they're infix ops as you mention). I made the changes you mention, updated my tests and all are passing.

The use case is to support MathQuill syntax, which here is an example on their doc website.

Screen Shot 2020-07-27 at 9 38 01 AM

Copy link
Owner

@arthanzel arthanzel left a comment

Choose a reason for hiding this comment

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

Looks good! I left some nitpicks but seems OK to merge after they're resolved.

@@ -9,6 +9,7 @@
"private": false,
"scripts": {
"build": "babel -d dist src && webpack dist/minifier.js -o dist/evaluatex.min.js",
"postinstall": "npm run build",
Copy link
Owner

Choose a reason for hiding this comment

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

Why the postinstall? The NPM package ships with Babel and minified ES sources. We don't want end users of the library to build it on every install.

// `b` isn't bound
test("{a}\\cdot{b}", undefined, { a: 2 }, {}, { latex: true });
} catch(err) {
assert.equal(true, err instanceof Error);
Copy link
Owner

Choose a reason for hiding this comment

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

This assertion should also check that the error message is correct and helpful, otherwise it passes on any error.

Chai also has an instanceOf assertion that would be more semantic here.

// `a` is a string
test("{a}\\cdot{b}", 4, { a: '2', b: 2 }, {}, { latex: true });
} catch(err) {
assert.equal(true, err instanceof Error);
Copy link
Owner

Choose a reason for hiding this comment

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

As above

@LennyLip
Copy link

Any chance you merge this?

@arthanzel
Copy link
Owner

Any chance you merge this?

Merged and published version 2.2.0

@arthanzel arthanzel closed this Nov 1, 2020
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.

3 participants