search

sutoiku / formula.js

JavaScript implementation of most Microsoft Excel formula functions
Other
2.14k stars 293 forks source link

Refactor #26

Closed joaojeronimo closed 10 years ago

joaojeronimo commented 10 years ago

We do not expect you to accept this PR but would like to offer it anyway. We did this major refactor so it would be easier to continue to contribute. Glad to answer any questions that arise. This is still a drop-in replacement for your module.

hmalphettes commented 10 years ago

@joaojeronimo, it looks fantastic. I'll make a PR on your branch to add the bessel* functions that we recently added: https://github.com/sutoiku/formula.js/issues/24 @jalateras what do you think? should we test-drive this in our product?

jalateras commented 10 years ago

@hmalphettes we should.

ghalimi commented 10 years ago

You guys rock!

http://sutoiku.com/post/94028279288/formulajs-graduated-today

On Thu, Aug 7, 2014 at 2:05 AM, João Jerónimo notifications@github.com wrote:

You can merge this Pull Request by running

git pull https://github.com/CrowdProcess/formula.js refactor

Or view, comment on, or merge it at:

https://github.com/sutoiku/formula.js/pull/26 Commit Summary

  • Add Grunt to make a bundle
  • Standalone Formula
  • Merge branch 'ADD'
  • Merge branch 'master' of github.com:CrowdProcess/formula.js
  • return #NUM! when POWER has a negative base and fractional exponent
  • fix conflict
  • Merge remote-tracking branch 'origin/error_handling'
  • check for isNaN too
  • merge upstream
  • Merge remote-tracking branch 'origin/flatten_values_in_IRR'
  • Merge branch 'flatten_ranges'
  • Merge branch 'master' of github.com:CrowdProcess/formula.js
  • major refactoring starting
  • remove expose.js
  • remove dependencies not needed anymore
  • add coveralls reporting
  • badges, reason for forking
  • correct branch in codeship, mixed up coveralls and coverage makes
  • some space
  • add default errors
  • add date function
  • update jStat
  • add utils
  • add parseBool
  • covered some math-trig functions
  • add compatibility aliases
  • add datevalue function
  • add day function
  • add days function
  • add days360 function
  • add edate function
  • add eomonth function
  • add hour function
  • add isoweeknum function
  • add minute function
  • lint separately
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • math and trig are 100% covered
  • now it's really 100% covered
  • typeof thing === 'undefined' is stupid
  • add month function
  • add networkdays function
  • add networkdays.intl function
  • add now function
  • add second function
  • add time function
  • add timevalue function
  • add today function
  • add weekday function
  • add weeknum function
  • add workday and workday.intl functions
  • add year function
  • add yearfrac function
  • almost done with statistical functions
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • expose parseDate in utils
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • test more
  • dots
  • statistical 100% covered and passing
  • WIP on finantial, almost done
  • test-watch
  • cover TBILL formulas
  • more coverage
  • add bessel functions
  • add bin2dec function
  • add bin2hex function
  • add bin2oct function
  • add bitand function
  • add bitlshift function
  • add bitor function
  • add bitrshift function
  • add bitxor function
  • add complex function
  • add convert function
  • add dec2bin function
  • add dec2hex function
  • add dec2oct function
  • add delta function
  • add erf function
  • add erf.precise function
  • add erfc function
  • add erfc.precise function
  • add gestep function
  • add hex2bin function
  • add hex2dec function
  • add hex2oct function
  • add imaginary function
  • add imreal function
  • add IMABS function
  • add imargument function
  • add imconjugate function
  • add imcos function
  • add imcosh function
  • add imsin function
  • add imcot function
  • add imdiv function
  • add imexp function
  • add imln function
  • add imlog10 function
  • add imlog2
  • add impower function
  • add improduct function
  • add imsec function
  • add imsech function
  • add imsinh function
  • add imsqrt function
  • add imsub function
  • add imsum function
  • add imtan function
  • add oct2bin function
  • add oct2dec function
  • add oct2hex function
  • remove ACCRINT, bad implementation
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • add and function
  • add false function
  • add if function
  • add iferror function
  • add ifna function
  • add not function
  • add or function
  • add true function
  • add xor function
  • do not limit IRR and XIRR, 100% coverage of financial functions
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • add error.type function
  • add cell function
  • add info function
  • add isblank function
  • add iserror function
  • add isformula function
  • add islogical function
  • add isna function
  • add isnontext function
  • add isnumber function
  • add isodd function
  • add isref function
  • add istext function
  • add n function
  • add na function
  • add sheet function
  • add sheets function
  • add type function
  • fix SUMPRODUCT case for uni dimensional arrays
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • finally 100% code coverage
  • oops

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/sutoiku/formula.js/pull/26.

jalateras commented 10 years ago

Once tested we should push a new release to the npm registry

cheers

On 7 Aug 2014, at 12:53 pm, Ismael Ghalimi notifications@github.com wrote:

You guys rock!

http://sutoiku.com/post/94028279288/formulajs-graduated-today

On Thu, Aug 7, 2014 at 2:05 AM, João Jerónimo notifications@github.com wrote:

You can merge this Pull Request by running

git pull https://github.com/CrowdProcess/formula.js refactor

Or view, comment on, or merge it at:

https://github.com/sutoiku/formula.js/pull/26 Commit Summary

  • Add Grunt to make a bundle
  • Standalone Formula
  • Merge branch 'ADD'
  • Merge branch 'master' of github.com:CrowdProcess/formula.js
  • return #NUM! when POWER has a negative base and fractional exponent
  • fix conflict
  • Merge remote-tracking branch 'origin/error_handling'
  • check for isNaN too
  • merge upstream
  • Merge remote-tracking branch 'origin/flatten_values_in_IRR'
  • Merge branch 'flatten_ranges'
  • Merge branch 'master' of github.com:CrowdProcess/formula.js
  • major refactoring starting
  • remove expose.js
  • remove dependencies not needed anymore
  • add coveralls reporting
  • badges, reason for forking
  • correct branch in codeship, mixed up coveralls and coverage makes
  • some space
  • add default errors
  • add date function
  • update jStat
  • add utils
  • add parseBool
  • covered some math-trig functions
  • add compatibility aliases
  • add datevalue function
  • add day function
  • add days function
  • add days360 function
  • add edate function
  • add eomonth function
  • add hour function
  • add isoweeknum function
  • add minute function
  • lint separately
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • math and trig are 100% covered
  • now it's really 100% covered
  • typeof thing === 'undefined' is stupid
  • add month function
  • add networkdays function
  • add networkdays.intl function
  • add now function
  • add second function
  • add time function
  • add timevalue function
  • add today function
  • add weekday function
  • add weeknum function
  • add workday and workday.intl functions
  • add year function
  • add yearfrac function
  • almost done with statistical functions
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • expose parseDate in utils
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • test more
  • dots
  • statistical 100% covered and passing
  • WIP on finantial, almost done
  • test-watch
  • cover TBILL formulas
  • more coverage
  • add bessel functions
  • add bin2dec function
  • add bin2hex function
  • add bin2oct function
  • add bitand function
  • add bitlshift function
  • add bitor function
  • add bitrshift function
  • add bitxor function
  • add complex function
  • add convert function
  • add dec2bin function
  • add dec2hex function
  • add dec2oct function
  • add delta function
  • add erf function
  • add erf.precise function
  • add erfc function
  • add erfc.precise function
  • add gestep function
  • add hex2bin function
  • add hex2dec function
  • add hex2oct function
  • add imaginary function
  • add imreal function
  • add IMABS function
  • add imargument function
  • add imconjugate function
  • add imcos function
  • add imcosh function
  • add imsin function
  • add imcot function
  • add imdiv function
  • add imexp function
  • add imln function
  • add imlog10 function
  • add imlog2
  • add impower function
  • add improduct function
  • add imsec function
  • add imsech function
  • add imsinh function
  • add imsqrt function
  • add imsub function
  • add imsum function
  • add imtan function
  • add oct2bin function
  • add oct2dec function
  • add oct2hex function
  • remove ACCRINT, bad implementation
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • add and function
  • add false function
  • add if function
  • add iferror function
  • add ifna function
  • add not function
  • add or function
  • add true function
  • add xor function
  • do not limit IRR and XIRR, 100% coverage of financial functions
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • add error.type function
  • add cell function
  • add info function
  • add isblank function
  • add iserror function
  • add isformula function
  • add islogical function
  • add isna function
  • add isnontext function
  • add isnumber function
  • add isodd function
  • add isref function
  • add istext function
  • add n function
  • add na function
  • add sheet function
  • add sheets function
  • add type function
  • fix SUMPRODUCT case for uni dimensional arrays
  • Merge branch 'refactor' of github.com:CrowdProcess/formula.js into refactor
  • finally 100% code coverage
  • oops

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/sutoiku/formula.js/pull/26.

— Reply to this email directly or view it on GitHub.

hmalphettes commented 10 years ago

Submitted a PR with the bessel functions here: https://github.com/CrowdProcess/formula.js/pull/1

Some of the tests are not passing on my machine due to overly precise number comparisons. The Travis build reports the same than what I see on my machine.

We will need to review in depth the removal of ACCRINT: https://github.com/CrowdProcess/formula.js/commit/390b6f7be1ea1a79fc23bf2ed56a65041f022c23

and the error handling: https://github.com/CrowdProcess/formula.js/commit/33e216a77cd5a5f09ba01ba06533fe5244e6fe6c

ghalimi commented 10 years ago

Why did we remove ACCRINT?

On Thu, Aug 7, 2014 at 2:55 PM, Hugues Malphettes notifications@github.com wrote:

Submitted a PR with the bessel functions here: CrowdProcess#1 https://github.com/CrowdProcess/formula.js/pull/1

Some of the tests are not passing on my machine due to overly precise number comparisons. The Travis build reports the same than what I see on my machine.

We will need to review in depth the removal of ACCRINT: CrowdProcess@390b6f7 https://github.com/CrowdProcess/formula.js/commit/390b6f7be1ea1a79fc23bf2ed56a65041f022c23

and the error handling: CrowdProcess@33e216a https://github.com/CrowdProcess/formula.js/commit/33e216a77cd5a5f09ba01ba06533fe5244e6fe6c

— Reply to this email directly or view it on GitHub https://github.com/sutoiku/formula.js/pull/26#issuecomment-51437394.

hmalphettes commented 10 years ago

@ghalimi "we" did not remove it. It is removed in CrowdProcess's branch.

In my understanding we are using 24 times the evil eval functions. CrowdProcess cannot afford to have such a security hole given their use case: running javascript on a distributed grid of browsers. So facing the choice between less supported formulas or bad-security they chose the safe way.

We can ask @joaojeronimo if there was something else going on.

ghalimi commented 10 years ago

I don't think I ever used eval in the original codebase. Somehow it must have been added by people who wanted some of the parameters to be more dynamic. This is obviously really bad, and a massive security hole. But this is not limited to ACCRINT. It's affectiving many financial functions. We'll need to clean that up...

hmalphettes commented 10 years ago

@ghalimi, I did not mean to point fingers and I'll stay away from the git blame command-line. Here is the issue to clean that up: https://github.com/sutoiku/formula.js/issues/27

joaojeronimo commented 10 years ago

Hey guys, very glad you didn't take this as a hostile fork and we can continue working together to make formula.js better :) :+1: for that.

@hmalphettes thanks for the bessel PR, we're just going to add input validation to it and merge it right away;

The tests not passing due to overly precise number assertions is indeed a problem that we are not sure on how to solve. Maybe make assertions to an interval instead of a fixed number ? Maybe use the cleanFloat function for assertions ?

We chose to remove ACCRINT because the current implementation was not correct and not taking into account the frequency parameter, as @ghalimi says here. By the way which is the Quantitative Finance library you talk about in the post ? I looked for it but only found https://github.com/lballabio/quantlib and couldn't find the algorithm there.

I found 2 common cases of eval usage:

So for now we have 8 uses of eval that we do not know how to get rid of, or even if that is possible, or even if that is desirable. But as for specific security concerns in CrowdProcess it's not a problem.

About the error handling, we feel that using instances of Error is more javascript idiomatic that returning strings that one must later compare with other strings to see if they are errors or just regular function output. That would make for instance the input of CONCATENATE("#NU", "M!") be a #NUM! error where it is not. When you use javascript Error instances, you can try possibleError instanceof Error to see if it's really an error, and then get what error it was with possibleError.message.

joaojeronimo commented 10 years ago

Update: I'm going to deal with the floating number assertions with sould.approximately, that's intended to solve the problem we're having. I didn't know about it before.

Update2: should.approximately solves most of the things but not string encoded imaginary numbers and arrays of floats. Going to try to parse the imaginary numbers and sum the arrays and then use approximately

Update3: used should.approximately everywhere so now the tests pass, merged the Bessel functions and started doing input treatment in all functions (parsing strings to numbers where possible, returning #VALUE! errors when numbers or string encoded numbers were mandatory). The coverage should drop until I test all the input treatment.

hmalphettes commented 10 years ago

@joaojeronimo thanks for the explanations. We will merge it eventually. Just more work on our plates and more reasons to do it.

joaojeronimo commented 10 years ago

In that case please don't merge it in the next hour or so because I'm doing all the type checking and input error handling. Btw we accidentally closed this branch and started committing to our master, so if you're thinking of merging it maybe we should do another pull request with the master or restore the refactor branch and merge what we have on the master branch, tell me if you prefer any of these two options or if they're the same to you.

jalateras commented 10 years ago

@joaojeronimo it would be awesome if you could open another pull request against master. That would surely expedite the release process.

joaojeronimo commented 10 years ago

Ok I will do that.

By the way currently the coverage is 94% instead of 100% because on friday I added proper input type checking and conversion to all the formulas and now I need to test all those cases, so I'll create the PR now but it should be merged after the coverage is back to 100%.

jalateras commented 10 years ago

thanks @joaojeronimo .I'll give a look out for the PR

joaojeronimo commented 10 years ago

@jalateras just finished covering the input parsing error cases, coverage is back to 100% and I'd say it's ready to be merged :)