Merge if you like math! #104
Conversation
composer.json
Outdated
There was a problem hiding this comment.
I would take out ext-bcmath as well since this requires one or the other. Might also want to look into adding them to suggested. What do you think?
There was a problem hiding this comment.
ext-bcmath is required because rpmath has not yet been integrated
There was a problem hiding this comment.
What if I have ext-gmp installed and don't have ext-bcmath?
There was a problem hiding this comment.
Both have been moved to suggested. Currently working on Rich's Math library
There was a problem hiding this comment.
git add composer.json && git commit -m "Moved to suggested" && git push
|
The only major thing I see at first glance is the Math class isn't tested very well and I can see some use cases that would cause things to break. What happens if I run $result = \Bitpay\Math\Math::add(1, 2);before I set an engine? |
|
Scratch that last comment, I see where you are setting a default engine. I'd still like to see more tests, looking at the code coverage, I wouldn't mind sitting down and helping do some. |
oops deleted an extra comma
|
Build fail, fix the build =) |
.travis.yml
Outdated
There was a problem hiding this comment.
you can probably remove these 3 arguments now.
|
Posted a few comments on the code, the more important thing is to make sure the build is passing. |
…alk about testMaths, Error.php causes build to fail
2 participants
GMP is no longer a dependency. If GMP does not exist, BC Math is used instead.