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

Added field of boolean type to config for enabling/disabling of implicit multiplication. #1502

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

nicholasnbg
Copy link

Issue: #1334

It's my first contribution to the project, please let me know if this is wrong, or if I've missed something.

Copy link
Collaborator

@ericman314 ericman314 left a comment

Choose a reason for hiding this comment

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

Could just leave the implicitMultiplication option undocumented until the logic is implemented in the parser.

@ericman314
Copy link
Collaborator

If you do want to implement the full logic of enabling/disabling implicit multiplication, this can help you get started.

The file you should be editing is https://github.com/josdejong/mathjs/blob/develop/src/expression/parse.js. The parser is structured as a chain of functions that get called recursively in order of increasing operator precedence. Each function matches its own part of the expression, then calls the next function.

Here is a small part of the chain:

  • parseAddSubtract(), which calls:
  • parseMultiplyDivide(), which calls:
  • parseImplicitMultiplication(), which calls:
  • parseRule2(), which calls:
  • parseUnary(), etc.

In order to disable implicit multiplication, the functions parseImplicitMultiplication() and parseRule2() would need to be skipped:

  • parseAddSubtract(), which calls:
  • parseMultiplyDivide(), which calls:
  • parseUnary(), etc.

Hope that helps :-)

@harrysarson
Copy link
Collaborator

Out of curiosity: what does parseRule2() do?

@ericman314
Copy link
Collaborator

@harrysarson it is in reference to this: #792 (comment)

@nicholasnbg
Copy link
Author

@ericman314 I'm happy to implement it as well, would you like that to be in this PR or a separate one?

@ericman314
Copy link
Collaborator

Thanks! I think you're good to do it in this PR.

@josdejong
Copy link
Owner

Thanks Nick! I agree with Eric, the actual enabling/disabling of implicit multiplication in parse.js can be part of this PR, these two parts are strongly dependent on each other.

Maybe change the description of the option to something like "Enable implicit multiplication in the expression parser. Default value is true".

Please don't forget to add some unit tests :)

@josdejong
Copy link
Owner

@nicholasnbg I see this PR is still open. Do you still plan on implementing this new option? It would be a nice addition.

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.

4 participants