search

eduolalo / moment-business-days

This is a momentJS plugin that allows you to use only business days (Monday to Friday)
MIT License
240 stars 67 forks source link

Support businessAdd with fractional units #80

Open waldyrious opened 4 years ago

waldyrious commented 4 years ago

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 commented 4 years ago

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?