My local test in rationalize.test.js using mocha is generating a weird error

SyntaxError: Unexpected token import

import core from './core/core'

I've researched in Internet and they say that Node.js does not accept import statement. However, I've tried some months ago and there was no problem! It is a kind of mistery

Now everything is OK. The only complain is that I don't get run Mocha in my local environment, so I should to rewrite rationalize.js just for I get debugging the code.

Thanks a lot for the fixes and improvements Paulo, looks good!

I don't fully understand isExactFract. Is this meant as a public option for simplify? If so, shouldn't we pass it via an object with options like simplify(expr, ..., {exactFractions: false}) instead of "disguised" as a rule?

I've researched in Internet and they say that Node.js does not accept import statement. However, I've tried some months ago and there was no problem! It is a kind of mistery

Sorry for the inconvenience. One month ago we released v5.0.0 where we changed the code from ES5 to ES6+, and set up build scripts to compile modern JS (in src) back to safe, old JS (in lib and dist). Thanks to the babel transpilation to ES5 we can use the newest javascript features, including import/export. However this isn't yet supported by node.js, which means we have to transpile everything too before we can run it in node.js. This includes running the unit tests: we first have to transpile to ES5, then we can run it on node.js. To do this, you have to pass an extra argument --require babel-core/register to mocha (see package.json):

mocha test test-node --recursive --require babel-core/register

Or simply run npm test of course, that will also run the linter that we introduced in mathjs v5 too :)

I don't fully understand isExactFract. Is this meant as a public option for simplify? If so, shouldn't we pass it via an object with options like simplify(expr, ..., {exactFractions: false}) instead of "disguised" as a rule?

Yes, you are right, @josdejong. I'll make the changes accordingly.

Done!

Your suggestion was much better than mine, but I had to change the parameter profile in simplify.js, that was long (12 different options ).

Now there is a new parameter in simplify function that can be used to put additional options. For now, it accepts only a boolean property: exactFractions. Default value is true, but rationalize uses false.

true value in exactFractions simplify 0.1*x to x/10, however false value get 0.1*x and keeps it..
This is referring to simplifyConstant function, which is part of the simplify standard rules.

I've added some calls to simplify.test.js in order to test the changes in simplify.js

Thanks @paulobuchsbaum , looks good like this, extending simplify with an options argument works out really nicely!

I'm not sure if you know this already, but you can run the linter locally via npm run lint, and it is also executed when running npm test.

Here a few feedback points:

1. I was wondering about the new added rules, do they belong in rationalize, or should we move them to simplify?

   {l: 'c1*n + n', r: '(c1+1)*n'}, // Joining constants
   {l: 'c1*n - c2*n', r: '(c1-c2)*n'}, // Joining constants
   {l: 'c1*n - n', r: '(c1-1)*n'}, // Joining constants
   {l: 'v/c', r: '(1/c)*v'}, // variable/constant (new!)
   {l: 'v/-c', r: '-(1/c)*v'}, // variable/constant (new!)
   

2. It's a minor thing, but I personally prefer variable names like options instead of short names like opts. I find it more descriptive and since we nowadays we have IDE's with autocompletion and plenty of RAM, a few bytes extra isn't a problem ;)

3. It may be more straigtforward and easier to extend later if we simply pass options to all rules, like:

   res = rules[i](res, options)

   instead of:

   if (rules[i] === simplifyConstant) {
     res = rules[i](res, exactFract)
   } else {
     res = rules[i](res)
   }

   what do you think?

4. Currently, the simplifyConstant handles the exactFraction option by moving it in a "global" constant (ie. defined outside of the function) when calling simplifyConstant, and indirectly using it in _toNumber. I think it will be more robust to pass it as an argument to _toNumber, so it's straigtforward to see which functions use/pass these options. And same as (3.) maybe we should simply pass options instead of exactFractions everywhere.

5. I think we should describe this new option in the docs of simplify, and probably create two simple extra examples demonstrating what exactFractions does. If you want I can help out here too and write the docs.

6. Thanks for working out unit tests for the new simplify options 👍 .

I try to change the code to make the options parameter local, but additionally I had to change some calls to foldFraction using map to for loops and also I try to change typed function _toNumber for a new profile with 2 parameters.

I had changed and added 31 lines (> 21 lines that I said above). I've realized that using options as a local variable passing down through tree of calls get hard to discover when the options parameter is effectively used and in any middle call (_eval(....)) it's a complete mistery the options meaning for any code reviewer.

In the end, many new errors have arisen and I've got so confused, that I've reverted all changes to previous state. I give in!

However, I think that it is very unlikely that new options will be necessary in simplifyConstant.js to the point of turning the code into spaghetti.

Another day, I will transfer the rules from rationalize.js to simplify.js default rules and propose an addendum in the documentation.

If all the auxiliary functions (_toNumber, _eval, foldFraction and _exactFraction) were internal to simplifyConstant function, its parameters could be used directly without assignment to a global variable. Even so, is this against your philosophy?

Thanks for giving it a try refactoring the options argument @paulobuchsbaum . I will merge your fixes now and give the refactoring a try too. If it indeed gives more issues then it solves I guess we should keep it as is. I will also describe options in the docs of simplify and add an example.

If all the auxiliary functions (_toNumber, _eval, foldFraction and _exactFraction) were internal to simplifyConstant function, its parameters could be used directly without assignment to a global variable. Even so, is this against your philosophy?

In mathjs we have that this situation for example with the dependency injection mechanism which passes typed and config and a some other things. In such a case I would say it's best to use the parameters that are already defined in the outer scope of the function and not pass them as argument to internal functions. I think the main reason for me would be that (a) these parameters are constants in the scope of this function, they will never ever change again unlike our current options "global", and (b) if you would pass them as arguments too, you would have two variables with the same name, shadowing each other, which is confusing. Does that sort of make sense?

I've refactored globalOptions into options, see e296fdc. There is about 30 occurances of options now, which is a lot.

I've realized that using options as a local variable passing down through tree of calls get hard to discover when the options parameter is effectively used and in any middle call (_eval(....)) it's a complete mistery the options meaning for any code reviewer.

I actually do like the transparency that I see after the refactoring: before, you had no clue that _eval actually needed options because one of the functions it calls needs options. Now you clearly see it.

I guess it's partly personal preference and I guess we simply have different opinions here, which is ok. Thanks for the constructive discussion and your flexibility.

Great! I've got the meaning of your refactoring. I've commit an error in apply usage. Thanks for all debate.

Nice work here 👍 These changes look great, I agree that parsing options around will make this code much easier for others to read and will make maintainance much smoother going forwards 😄

@paulmasson your fixes and improvements are now available in v5.1.0. Thanks again for all your effort!

You’d be welcome, but I think you mean @paulobuchsbaum

whoops 😳, sorry Paul. I mean @paulobuchsbaum instead

☺️, thank you, @josdejong, for the opportunity to interact and learn.