spatie / period

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

Add Duration #26

Closed jeromegamez closed 5 years ago

jeromegamez commented 5 years ago

Another attempt to add a Duration object that can be used together with a Period.

Addresses #18, #22, #25

Added components/behaviour

I hope the DurationTest class is an adequate show case for what's possible (a.k.a. I'm too lazy to copy paste code examples 😅)

Caveats

I don't think there is a way to reliably support variable precisions. To cite what I wrote in https://github.com/spatie/period/issues/18#issuecomment-458887498:

A duration (and for that matter a DateInterval) is a relative value, while a Period is based on absolute days. That causes problems when comparing periods like (2018-03-01, 2018-03-31) and (2018-04-01, 2018-04-30). When representing both periods as Duration/DateInterval, the value would be P1M (= 1 month). So while these two are equal in terms of being a full month, they are not in terms of the number of included days. Similar problem with weeks/days in a week.

That's why (at the moment) I let Duration throw an InvalidArgumentException when a value with a precision larger than Precision::DAY is provided. I hope I'm wrong, so please correct me :)

:octocat:

brendt commented 5 years ago

Such a great PR! One general remark, and I'm sorry for this: we have the convention at Spatie never to use private, final and void 🙈While I personally think it's the right way to use them, I also agree that there's value in company guidelines. So I'm sorry to ask you to remove voids and finals, and make everything at least protected.

Also: let's think about the month and year problem before merging this.

brendt commented 5 years ago

Update! We've adjusted our guidelines, to allow private, void and final. Do you have the time to add them @jeromegamez ? If not I'll be happy to make the changes.

I would also like to move forward with merging this PR, but I believe there's still one issue to be addressed, here's what I wrote in a comment:


Can we, since durations are always saved as seconds, only compare these amounts of seconds to each other?

If we want to use durations to be able to compare two period lengths with different precisions, it makes sense to convert everything to the smallest precision unit, and make comparisons with those values.

Let's take your example:

$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());

The duration of $a is 2678400 seconds, if I'm not mistaken. $b has the same amount of seconds, regardless of its precision, hence their durations would be equal. We should note in the readme that this doesn't necessarily mean that the two periods are equal, but that's the whole point of adding this functionality: being able to more loosely compare two periods.

Doesn't this eliminate the whole month issue?

jeromegamez commented 5 years ago

I was happy when I read about your updated guidelines, just didn't get around to update the PR, but here I am 🎉

I just reverted the "Unprivatize" commit and will add the checks for seconds as you suggested. If we run into problems with this in the future, we certainly can tackle them then (I'm constantly overthinking to avoid any problems in the future and have to remind myself regularly that this thwarts progress ¯_(ツ)_/¯)

brendt commented 5 years ago

Awesome! Looking forward to merging this PR

jeromegamez commented 5 years ago

Closing in favor of #32

brendt commented 5 years ago

🎉