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

Support businessAdd with fractional units #80

Open
waldyrious opened this issue Oct 22, 2019 · 1 comment
Open

Support businessAdd with fractional units #80

waldyrious opened this issue Oct 22, 2019 · 1 comment

Comments

@waldyrious
Copy link

Previously businessAdd() used to go into an infinite loop if a fractional value was passed, e.g. businessAdd(1.5, 'days'). That was reported as #55 and fixed in #59.

However, the fix done in #59 simply rounds the passed values, which may not be desired: businessAdd(1.5, 'days') silently becomes businessAdd(2, 'days'), which is probably not the expected result and could cause subtle bugs.

Since it's possible to add periods other than days, I think it would make sense for businessAdd() to actually support fractional units, so that, for instance, businessAdd(1.5, 'days') would have the same effect as businessAdd(24+12, 'hours').

I'd be glad to contribute a fix, but I'm not sure what would be the best way to do this. Would converting the passed units to, say, seconds, and rounding at that scale be acceptable? Or is there a cleaner way to subdivide a fractional moment.duration into its components and add them separately using businessAdd with the appropriate units?

@mcdado
Copy link
Collaborator

mcdado commented Nov 4, 2019

Would converting the passed units to, say, seconds, and rounding at that scale be acceptable?

I would say even minutes... i mean, when considering calculations on business days, with timezones, DST, etc, I would say that minutes is a good line in the sand. Maybe there are gonna be uses cases where seconds might be useful, that sounds a bit overkill for this plugin...

As to the most appropriate procedure... I would say first convert the period to minutes, then add, finally if the result is on a non-business day, add one more day. Sounds correct?

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

No branches or pull requests

2 participants