Fixes #76

[ameenross]

• let diff just straight up call diffSingle in a loop, remove the special case for a single non-overlapping period
• adjust test case

[ameenross]

sigh Am I missing something or do those failing tests make no sense either?

[ameenross]

So after looking at some documentation again (diffSingle has nothing in the readme, only in the tests), it seems to me that diff and diffSingle are substantially different.

// diffSingle test from PeriodTest::it_can_determine_the_diff_if_periods_do_not_overlap_at_all
/**
 * @test
 *
 * A                    [=====]
 * B        [=====]
 *
 * DIFF     [=====]     [=====]
 */

// diffSingle test from PeriodTest::it_can_create_a_diff_for_two_periods
/**
 * @test
 *
 * A        [===========]
 * B            [===========]
 *
 * DIFF     [==]         [==]
 */

// diff example from Readme
/**
 * A                   [====]
 * B                               [========]
 * C         [=====]
 * CURRENT      [========================]
 *
 * DIFF             [=]      [====]
 */

As you can see, the diffSingle function apparently means an XOR of sorts, but the diff function means a subtract. All of A, B and C are subtracted from CURRENT. If this was like diffSingle, XOR, but with multiple periods to XOR against, then the start of period C should also have been returned. That's not the case.

So as the diffSingle function is not in the readme yet, is this simply an oversight? Or is the way it works right now by design? If the former, then there are obviously some bugs (and tests) to fix, and if the latter, then I would really argue the functions need different names.

[brendt]

Sorry for the delay in replying! I'll look at this in depth next week :)

[brendt]

Hi @ameenross

So as the diffSingle function is not in the readme yet, is this simply an oversight? Or is the way it works right now by design?

It's by design.

then I would really argue the functions need different names.

I agree, which is why I started a new v2 branch a long time ago that tries to address the naming issues (https://github.com/spatie/period/blob/2.0/src/Period.php)

You'll see diffSingle is still used though, because I wasn't sure about what to call it instead. Do you have any recommendations?

I'll close this PR but feel free to further discuss the naming issue.

[ameenross]

I must say I'm unpleasantly surprised. My solution might not be right for some reason, but the following behavior surely cannot be right!

// Period for entire month of January.
$january = Period::make('2021-01-01', '2021-01-31'));

// Diff with march.
$diff = $january->diff(
    Period::make('2021-03-01', '2021-03-31')
);

// February??
$diff[0]->getStart()->format('Y-m-d'); // 2021-02-01
$diff[0]->getEnd()->format('Y-m-d');   // 2021-02-28
count($diff);                          // 1

/** Doing the exact same thing, but remove a second, bogus, period as well. */

$diff = $january->diff(
    Period::make('2021-03-01', '2021-03-31'),
    Period::make('9999-12-01', '9999-12-31')
);

// January, as expected.
$diff[0]->getStart()->format('Y-m-d'); // 2021-01-01
$diff[0]->getEnd()->format('Y-m-d');   // 2021-01-31
count($diff);                          // 1

[ameenross]

I've had to program this ugly workaround:

// Workaround for https://github.com/spatie/period/issues/76.
if (count($periodsToDiffWith) == 1) {
    $periodsToDiffWith[] = new Period(
        new DateTimeImmutable('9999-01-01'),
        new DateTimeImmutable('9999-01-02'),
        Precision::SECOND,
        Boundaries::EXCLUDE_END
    );
}

Don't tell me this peculiar behavior is by design?

[ameenross]

discuss the naming issue.

About that, diffSingle clearly isn't simply the n=1 variant of the diff function. The way it works needs to be documented (this needs to be done regardless). Then based on that it shouldn't be too hard to think of a good name. In set theory, the way diffSingle works is called a symmetric difference. Based on that, you could go for diff and symmetricDiff or something. If that sounds too technical, perhaps diff (symmetric) and subtract.

[brendt]

I appreciate you thinking along, but you should realise this package is used by many and we can't assume your use case applies to all other people.

I agree that the names are confusing, but we can't change them without a new major release. I do really like subtract. I'll try my best to finish v2 with proper renames, and you're welcome to discuss those changes on the PR once I send it for review.

I also like diffSymmetric btw, I'll give it some thought

[ameenross]

we can't assume your use case applies to all other people

Bug or feature? In my opinion, if you wanted a gap you should have asked for the gap, not the diff. So wrong usage (you're holding it wrong) and therefore bug. But I digress, I'm happy to see this fixed in 2.0...

I also like diffSymmetric btw, I'll give it some thought

Yeah, so it's good because it avoids the somewhat ambiguous meaning of diff in this context. Also, maybe if someone is really after that, then they probably know about set theory. In any case it's a more advanced feature than simple union/intersection/difference.