spatie / period

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

Add conversion from and to native DatePeriods #50

Closed jeromegamez closed 4 years ago

jeromegamez commented 4 years ago

As proposed in the conversation around #44, this PR adds methods to create a Period from a native DatePeriod and vice versa.

The conversions are based on the assumption that if I create a Period with an exclusion parameter other than Boundaries::EXCLUDE_NONE, I'm not interested in the exclusion (otherwise, Period::contains() wouldn't return false).

use Spatie\Period\Boundaries;
use Spatie\Period\Period;
use Spatie\Period\Precision;

$period = Period::make(
    '2018-01-01',
    '2018-01-07',
    Precision::DAY,
    Boundaries::EXCLUDE_START
);

assert($period->contains(new DateTime('2018-01-01')) === false);

Accordingly, converting a native DatePeriod to a Period, followed by a conversion from a Period to a native DatePeriod doesn't necessarily mean that the output is equal to the input:

$nativeInputPeriod = new DatePeriod(
    new DateTime('2018-01-01'),
    new DateInterval('P1D'),
    new DateTime('2018-01-07'),
    DatePeriod::EXCLUDE_START_DATE
);

$period = Period::fromDatePeriod($nativeInputPeriod);

assert($period->contains(new DateTime('2018-01-01')) === false);

$nativeOutputPeriod = $period->asDatePeriod();

assert($nativeOutputPeriod->include_start_date === true);
assert($nativeOutputPeriod->getStartDate() == new DateTime('2018-01-02'));

This is targeting a 1.x release because it's only added functionality without breaking changes.

:octocat:

kylekatarnls commented 4 years ago

Perfect! I was just about to propose a PR for that.

kylekatarnls commented 4 years ago

I understand the point: preferring having matching start/end than matching iteration items. I agree and will follow that for my mixin.

jeromegamez commented 4 years ago

I believe having the ::fromPeriod constructor makes sense, people coming to this library are probably at a point where the default DatePeriod doesn't cut it anymore and want to go further. Providing an easy entry point to convert their existing DatePeriods to this one seems like a nice thing to do 😅

On the other hand, this would probably also mean that they could then expect to do something like e.g. $spatiePeriod->overlapsWith($nativeDateperiod), which would probably mean that we would have to loosen up the methods by not type-hinting the parameters and having to do

$period = $period instanceof Period ? $period : Period::fromDatePeriod($period);

in case we decided to support that 🤔 (where's the :sigh: emoji ^^)

brendt commented 4 years ago

which would probably mean that we would have to loosen up the methods by not type-hinting the parameters

It's good that this is possible, but let's not concern the Period class with it. If users are using a DatePeriod, they should convert it to Period from the outer context.

jeromegamez commented 4 years ago

Alright! 👍

brendt commented 4 years ago

Oh hang on, I didn't mean you had to close the PR, I still want to add this! I just don't want to open up all comparison methods to also support DatePeriod

jeromegamez commented 4 years ago

Ah 😅! Then I can now reply to the review comments :)

jeromegamez commented 4 years ago

FYI/FYE, nice edge case:

$period = new DatePeriod(
    new DateTime('2018-01-01'), 
    new DateInterval('PT0S'),
    new DateTime('2018-01-02')
);

foreach ($period as $date) {
    // Infinite amount of dots
    echo '.';
}
spatie-bot commented 4 years ago

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.