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

updated Duration.milliseconds() to return an integer, and added floating point math #4787

Closed
wants to merge 3 commits into from
Closed

Conversation

martin770
Copy link

I updated duration.milliseconds() to truncate partial milliseconds and return an integer to fix issue #4649 while allowing duration.asMilliseconds() to still return a non-integer number.

As I was adding test cases for it, I noticed that some of the calculations were incorrect due to javascript floating point math problems. See: #2978 and #1867

I added a function (fpMath) to correctly perform floating point calculations, and added test cases around that to make sure everything is behaving as expected. I did not have to modify any existing test cases, so this should not negatively affect any existing functionality.

I only modified files in /src/ and all the tests are passing, but I wasn't sure if I need to also make the changes to /moment.js also or if that file gets built from src somehow.

@jsf-clabot
Copy link

jsf-clabot commented Sep 24, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


James Martin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@martin770
Copy link
Author

You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

I signed the CLA, but it says my e-mail address is not linked to my github account. I checked my profile, and it's there...

@marwahaha
Copy link
Member

@martin770 - your git commits aren't with the same email address, please update them

@martin770 martin770 closed this Jan 30, 2019
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.

3 participants