briannesbitt / Carbon

A simple PHP API extension for DateTime.
https://carbon.nesbot.com/
MIT License
16.57k stars 1.28k forks source link

Allow passing floats to `CarbonInterval` helpers #1901

Closed atymic closed 5 years ago

atymic commented 5 years ago

Hello,

I encountered an issue with the following code:

echo CarbonInterval::hours(3.5);

Carbon version: 2.24.0

PHP version: 7.3

I expected to get:

A CarbonInterval with 3 hours and 30 minutes.

But I actually get:

An exception due to the invalid format

Error: DateInterval::__construct(): Unknown or bad format (PT3.5H)

I was wondering if this would be something worth supporting (floats instead of ints).

I know if differs a bit from the underlying implementation, but I think it would be helpful to accept floats and handle the conversions internally.

I can have a go at a PR if you like the idea :)

Thanks!

kylekatarnls commented 5 years ago

Hello,

CarbonInterval extends DateInterval native PHP class. And this native class handle each unit separately, it does not guess the cascading. The main reason is: while you can easily assume 1 hour is 60 minutes and 1 minute is 60 seconds (except leap second: https://en.wikipedia.org/wiki/Leap_second), it start to become harder with day (that can be 23, 24 or 25 hours due to DST transitions) then months can be 28, 29, 30 or 31 days. So there is no possible generic cascading: 1.5 day = ambiguity in DST change days (and intervals has no calendar positioning in it), then 1.5 month is impossible to calculate if you don't take an approximation assertion.

That's why we provide a cascade() method to be called manually rather than automatic stuff:

CarbonInterval::seconds(3.5 * 3600)->cascade() // 3 hours and 30 minutes

This also limit the rounding, for example, if you ask for 1/7 hour, you get a different result if you set via CarbonInterval::minutes(round(1 / 7 * 60)), CarbonInterval::seconds(round(1 / 7 * 3600)) or CarbonInterval::microseconds(round(1/7 * 3600000000)) you can choose explicitly the smallest unit you want as precision while with CarbonInterval::hours(1 / 7) you can't really guess if you will have minutes, seconds, ms or ยตs precision.

I'm open to discussion but before a PR, I would like to know about specifications and edge cases (small/big numbers, units behavior, precision, etc.)

atymic commented 5 years ago

Thanks for the great explanation. Your answer makes complete sense, I didn't think about the complexity on other units.

I think that using cascade and setting the units is fine, so i'll close the issue ๐Ÿ˜„