Fix infinite loop caused by fraction value in businessAdd (fixes #55) #59
Conversation
Fix: Fractional day causes infinite loop in businessAdd eduolalo#55
Added test for Fractional day causes infinite loop in businessAdd eduolalo#55
mcdado
changed the title
|
Hi, thanks for sending the PR! I see that there are problems with whitespace causing the whole file to diff (you can see it by going to the "Files changed" tab in this PR page and on the right open "Diff settings" and select "Hide whitespace changes". Anyway, I'll review it and in case fix the problems 😉 |
index.js
Outdated
| return day; | ||
| } | ||
| var signal = number < 0 ? -1 : 1; | ||
| var remaining = Math.abs(Math.trunc(number)); |
There was a problem hiding this comment.
I'm not sure if this is correct, since it doesn't do any rounding.
Moment.js has a specific function for rounding absolute values, take a look:
https://github.com/moment/moment/blob/master/src/lib/utils/abs-round.js
What do you think? Is it worth it to use that four-liner to handle this case here too?
In this specific case the problem was due to line endings… your commit switches to CRLF (Windows style) instead of the default LF (Unix Style). Do you mind either fixing it or even better force push to |
|
I think it was my error causing the whole file diff. I'm a github newbie
:/ Thanks for your patience; )
…On Thu, Nov 29, 2018, 5:01 AM David Gasperoni ***@***.*** wrote:
Hi, thanks for sending the PR! I see that there are problems with
whitespace causing the whole file to diff (you can see it by going to the
"Files changed" tab in this PR page and on the right open "Diff settings"
and select "Hide whitespace changes".
Anyway, I'll review it and in case fix the problems 😉
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#59 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHuoFk5ZFuWnN5Mtr67U0IvZcXKZuyQgks5uz76EgaJpZM4Y4f-l>
.
|
Hey we all were newbie at some point, I sure made similar mistakes in the beginning! 😉 |
|
Yes, please fix as necessary.
…On Mon, Dec 3, 2018 at 3:04 AM David Gasperoni ***@***.***> wrote:
I think it was my error causing the whole file diff. I'm a github newbie
:/ Thanks for your patience; )
Hey we all were newbie at some point, I sure made similar mistakes in the
beginning! 😉
About the diff, as I mentioned the problem is with line endings. If you
don't mind I can fix both problems for you and add the absolute value
rounding.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#59 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHuoFtTO6dbFPOCOI_lwmoAgbPN7jt4yks5u1OkOgaJpZM4Y4f-l>
.
|
|
Thanks guys, I've deployed it to NPM as V1.1.3. |
|
Was there a reason why you reset the original date's timestamp when do you We feel like it might affect the whole project, so are sticking with <1.1.3 for the time being. |
|
Hi Renan, sorry, that wasn't intentional. I just added a PR to fix.
Jerry
…On Wed, Dec 5, 2018, 4:41 PM Renan Martins ***@***.*** wrote:
Was there a reason why you reset the original date's timestamp when do you
businessAdd()? I have a project that relies on it, some test started
failing and we realized this new version now resets the timestamp because
of this line
https://github.com/kalmecak/moment-business-days/pull/59/files#diff-168726dbe96b3ce427e7fedce31bb0bcR74
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#59 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHuoFtfeo73HeSNAYOFxDtooQyYIx1q3ks5u2EuNgaJpZM4Y4f-l>
.
|
Using businessAdd does not reset the time of day.
Sorry, this slipped through review. Do you have some of those tests to contribute this library? That would be great. @avenue19 did add one, the more the better I guess… 😅 |
…ing timestamps in businessAdd (eduolalo/moment-business-days#59)
Sorry, my first PR and I uploaded one entire file instead of editing on web. I wasn't able to push from my local clone due to proxy issue.