spatie / period

Complex period comparisons
https://spatie.be/open-source
MIT License
1.59k stars 72 forks source link

Add PeriodDuration #32

Closed jeromegamez closed 5 years ago

jeromegamez commented 5 years ago

This is an alternative approach to #26 and tries to address the challenge that Period durations were only reliably usable with a precision up to Precision::DAY.

In #26, a Duration is a standalone Value Object, not connected to a Period it might have been created from, and its state is represented by the number of seconds that it is made of.

With #26, creating a duration from a Period would have resulted e.g. in the following values:

$a = Period::make('2018-01-01', '2018-01-31', Precision::MONTH); // P1M
$b = Period::make('2018-02-01', '2018-02-28', Precision::MONTH); // P1M

$a->duration()->equals($b->duration()); // false, because the number of seconds doesn't match

A previous iteration of #26 used a DateIntervalto represent the duration, but in that case, something like this wouldn't work:

$a = Period::make('2018-01-01', '2018-01-31', Precision::MONTH); // P1M
$b = Period::make('2018-01-01', '2018-01-31', Precision::DAY); // P31D

$a->duration()->equals($b->duration()); // false, because the DateInterval is different

The advantage of #26 is that you can do Duration::length(Precision $precision) to get the actual value of the duration. This is not possible here, because here, different possibilities of "equality" can be considered, so that the following examples all return true when calling $period->duration()->equals() with each other.

$a = Period::make('2018-01-01', '2018-01-31'); // P1M + P31D ?
$b = Period::make('2018-02-01', '2018-02-28'); // P1M + P28D ?
$c = Period::make('2020-03-01', '2020-03-28'); // P28D

// The following could be considered equal because ->getStart() and getEnd() return the same date
$d = Period::make('2018-01-01', '2018-01-31', Precision::DAY, Boundaries::EXCLUDE_ALL);
$e Period::make('2018-01-01', '2018-01-31', Precision::DAY, Boundaries::EXCLUDE_NONE);

I'm sure I missed edge cases, but I'm covering all that I could think of in the tests.

As this library is about comparisons, perhaps getting an actual value from the duration is not so important, perhaps I'm wrong and perhaps I'm just going crazy 😅 - for the past week, I've spent almost every evening trying to wrap my head around this in dozens of local branches and always found something that didn't work out - so this is my last attempt. If #26 or this one here doesn't make sense, it's perhaps not meant to be 😅

:octocat:

brendt commented 5 years ago

Yes! This is the winner! Anything you'd like to change still before merging this into the 2.0 branch?

PS: I really appreciate the effort you put into this Jérôme, thank you very much!

jeromegamez commented 5 years ago

If you're fine with it, I certainly am 😌🎉. I just added a commit to fix some copy pasted comments in the tests, but besides that I'm happy you like it!

brendt commented 5 years ago

The only thing missing is a short explanation about how durations are compared in the README. After that I'm fine with merging this in!

jeromegamez commented 5 years ago

While adding to the README I'm wondering if the isSameAs() method actually makes sense for a duration. It just compares the properties of the underlying Periods instead of comparing something duration related - it would probably fit better in the Period itself (not part of this PR tho :D)

Would you be okay if I remove that again? This would leave us with equals(), isSmallerThan() and isLargerThan().

brendt commented 5 years ago

Sounds good!

jeromegamez commented 5 years ago

Done! I'll squash the commits once the tests pass and then I won't touch this branch anymore 😅

brendt commented 5 years ago

Feel free to merge it

jeromegamez commented 5 years ago

🎉