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.

Sign up for GitHub

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

Jump to bottom

Refactor #26

Closed
wants to merge 149 commits into from
Closed

Refactor #26

wants to merge 149 commits into from

Conversation

joaojeronimo
Copy link
Contributor

  • Separate formulas in their respective categories;
  • Fix several bugs;
  • Cover 100% of the code with tests;
  • Remove the website from this repository, should be separate;
  • Write tests with mocha, no generator (more flexible);
  • Remove moment and underscore.string dependencies;
  • Different error handling

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.

joaojeronimo and others added 30 commits June 26, 2014 16:00
@joaojeronimo
Add Grunt to make a bundle
9dae096
@joaojeronimo
Standalone Formula
d7d868c
@joaojeronimo
Merge branch 'ADD'
b1c9528
@joaojeronimo
Merge branch 'master' of github.com:CrowdProcess/formula.js
4a8dbe7
@joaojeronimo
return #NUM! when POWER has a negative base and fractional exponent
a26c61d
@joaojeronimo
fix conflict
ba265a0
@joaojeronimo
Merge remote-tracking branch 'origin/error_handling'
8e053df
@joaojeronimo
check for isNaN too
43b08df
@joaojeronimo
merge upstream
4c82231
@joaojeronimo
Merge remote-tracking branch 'origin/flatten_values_in_IRR'
f76e1cf
@joaojeronimo
Merge branch 'flatten_ranges'
2593a9b
@joaojeronimo
Merge branch 'master' of github.com:CrowdProcess/formula.js
6abb47c
@joaojeronimo
major refactoring starting
b564bae
@joaojeronimo
remove expose.js
e93da22
@joaojeronimo
remove dependencies not needed anymore
a56a82d
@joaojeronimo
add coveralls reporting
ce6c51e
@joaojeronimo
badges, reason for forking
deb4c60
@joaojeronimo
correct branch in codeship, mixed up coveralls and coverage makes
4ecc993
@joaojeronimo
some space
5ba57b4
@miguelvps
add default errors
33e216a
@miguelvps
add date function
6ceb391
@joaojeronimo
update jStat
d77fbd2
@joaojeronimo
add utils
b711731
@joaojeronimo
add parseBool
a292588
@joaojeronimo
covered some math-trig functions
951b4a2
@joaojeronimo
add compatibility aliases
6ae1bee
@miguelvps
add datevalue function
a1856d7
@miguelvps
add day function
a4a47aa
@miguelvps
add days function
d08e89e
@miguelvps
add days360 function
47dc24c
@hmalphettes
Copy link
Contributor

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

@jalateras
Copy link
Contributor

@hmalphettes we should.

@ghalimi
Copy link
Contributor

ghalimi commented Aug 7, 2014

You guys rock!

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

On Thu, Aug 7, 2014 at 2:05 AM, João Jerónimo [email protected]
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:

#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
#26.

@jalateras
Copy link
Contributor

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

cheers

On 7 Aug 2014, at 12:53 pm, Ismael Ghalimi [email protected] 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 [email protected]
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:

#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
#26.

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

@hmalphettes
Copy link
Contributor

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
Copy link
Contributor

ghalimi commented Aug 7, 2014

Why did we remove ACCRINT?

On Thu, Aug 7, 2014 at 2:55 PM, Hugues Malphettes [email protected]
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
#26 (comment).

@hmalphettes
Copy link
Contributor

@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
Copy link
Contributor

ghalimi commented Aug 7, 2014

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
Copy link
Contributor

@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: #27

@joaojeronimo
Copy link
Contributor Author

Hey guys, very glad you didn't take this as a hostile fork and we can continue working together to make formula.js better :) 👍 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:

  • to treat function inputs (accept strings that may be converted to integers). We removed these because we feel it's not a good pattern. In the cases where excel really allowed integers to be passed as strings, we made the proper input treatment without eval (and we tested every function that it's covered in excel to make sure of the behaviors)
  • for comparisons in formulas like SUMIF, here since people really input things like ">5" in excel, we did not remove eval in this case. However in functions that made multiple evals like SUMIFS and COUNTIFS, we concatenated all the conditions in one bigger condition string and evalled only once, for performance reasons.

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
Copy link
Contributor Author

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.

@miguelvps miguelvps deleted the refactor branch August 7, 2014 17:14
@hmalphettes
Copy link
Contributor

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

@hmalphettes hmalphettes closed this Aug 8, 2014
@joaojeronimo
Copy link
Contributor Author

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
Copy link
Contributor

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

@joaojeronimo
Copy link
Contributor Author

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%.

@joaojeronimo joaojeronimo mentioned this pull request Aug 11, 2014
Merged
@jalateras
Copy link
Contributor

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

@joaojeronimo
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@joaojeronimo @hmalphettes @jalateras @ghalimi @miguelvps