Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2 #80

Merged
merged 43 commits into from
Mar 17, 2021
Merged

v2 #80

merged 43 commits into from
Mar 17, 2021

Conversation

brendt
Copy link
Collaborator

@brendt brendt commented Feb 25, 2021

  • Bump required PHP version to ^8.0
  • Fix bug with overlapAll when no overlap
  • Period::duration() returns an instance of PeriodDuration
  • Period::length() now uses the Period's precision instead of always returning days
  • All period properties are now typed
  • Return types of several methods have been changed from Period to static
  • PeriodCollection::overlap() now accepts one or several periods
  • Period::overlap() renamed to Period::overlapAny()
  • Period::overlapSingle() renamed to Period::overlap()
  • Period::diff() renamed to Period::subtract()
  • Period::subtract() (previously diff) no longer returns the gap when there's no overlap
  • Period::diffSingle() renamed to Period::diffSymmetric()
  • Period::contains() now accepts both DateTimeInterface and Period

@brendt
Copy link
Collaborator Author

brendt commented Feb 25, 2021

@ameenross I'd appreciate your input on this one. diff has been renamed to subtract and diffSingle simple to diff. Furthermore, subtract doesn't gap anymore if there's no overlap.

@brendt
Copy link
Collaborator Author

brendt commented Feb 25, 2021

@jeromegamez it took a looooong time but I think of moving this forward. Would you mind taking a look at the changes?

@brendt
Copy link
Collaborator Author

brendt commented Feb 25, 2021

TODO:

@brendt brendt changed the title 2.0 v2 Feb 25, 2021
@ameenross
Copy link

ameenross commented Feb 25, 2021

Thinking about this again, I think the diff function should be done away with completely. For 2 reasons:

  1. for people who know set theory, diff might still be confused for a regular diff
  2. as it stands, the diff function completely changed meaning from v1 to v2 and will confuse many devs

So I say it should be subtract and diffSymmetric. Then people who used diff will find it no longer exists and have to make a conscious decision between the 2.

@panda-madness
Copy link

Correct me if I'm wrong, but skimming through the PR I don't see why PHP support couldn't be 7.4 | 8?

@brendt
Copy link
Collaborator Author

brendt commented Feb 25, 2021

@panda-madness

our projects are on PHP:^8.0 and we want to help move the community forward. No worries: the v1 version will still be here if you need legacy support.

Oh and I'm also reworking towards PHP 8, but haven't pushed those changes yet. So from v2 this package will use promoted properties, match, static return type etc.

@panda-madness
Copy link

panda-madness commented Feb 25, 2021

If more usage of PHP 8 features are coming then I don't have any counterpoints.

On a more general note, I don't really agree with bumping versions up just for the sake of it. IMO wider support eases new PHP version adoption in the long run. But it's your package, so you get to pick the music.

@brendt
Copy link
Collaborator Author

brendt commented Feb 25, 2021

We've been applying this strategy for years now at Spatie. Experience and practice shows that it works, and helps us stay up to date with the growing PHP community.

Copy link
Contributor

@jeromegamez jeromegamez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to test it for real since I'm currently not set-up with multiple PHP Versions on my computer and still on PHP 7.4, but looks good!

.php_cs Show resolved Hide resolved
src/PeriodDuration.php Outdated Show resolved Hide resolved
src/PeriodDuration.php Outdated Show resolved Hide resolved
Comment on lines 39 to 41
$now = new DateTimeImmutable('@'.time()); // Ensure a TimeZone independent instance

$here = $this->period->getIncludedEnd()->diff($this->period->getIncludedStart(), true);
$there = $other->period->getIncludedEnd()->diff($other->period->getIncludedStart(), true);

return $now->add($here)->getTimestamp() <=> $now->add($there)->getTimestamp();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have access to $here->period and $there->period, what would you think about using the start dates of them instead of the absolute $now (which can be outside of the inspected periods and lead to edge cases that I can't see at the moment but am sure exist #leapyears 😅), for example something like:

Suggested change
$now = new DateTimeImmutable('@'.time()); // Ensure a TimeZone independent instance
$here = $this->period->getIncludedEnd()->diff($this->period->getIncludedStart(), true);
$there = $other->period->getIncludedEnd()->diff($other->period->getIncludedStart(), true);
return $now->add($here)->getTimestamp() <=> $now->add($there)->getTimestamp();
$here = $this->period->getIncludedEnd()->diff($this->period->getIncludedStart(), true);
$there = $other->period->getIncludedEnd()->diff($other->period->getIncludedStart(), true);
return $here->period->getIncludedStart()->add($here)->getTimestamp() <=> $there->period->getIncludedStart()->add($there)->getTimestamp();

@brendt
Copy link
Collaborator Author

brendt commented Feb 25, 2021

FYI I added some diagrams to the README to clarify all period operations: https://github.com/spatie/period/tree/2.0

@ameenross
Copy link

I want to throw in another suggestion here.

PHP DateTime has supported microseconds for some time now. How about a "precision" is added called ARBITRARY?

@brendt
Copy link
Collaborator Author

brendt commented Feb 26, 2021

Why ARBITRARY and not MICROSECONDS ?

@ameenross
Copy link

ameenross commented Feb 26, 2021

Why ARBITRARY and not MICROSECONDS ?

Because I would say, at some point people stop caring about precision and just call it arbitrary precision. And I think the precision is a language thing more than anything. My mind was wandering a bit when I was thinking about how to use Period vs how I would have built it. Maybe you will understand better when I tell you a bit about those thoughts. Consider something like this:

// 2020-01-01 00:00:00 <= X < 2021-01-01 00:00:00
PeriodFactory::fromDate(new DateTime('2020-01-01 13:05'))->toDateInclusive(new DateTime('2020-12-31'));

// 2020-07-01 13:05:00 <= X < [now]
PeriodFactory::from(new DateTime('2020-07-01 13:05'))->to(new DateTime);

// 2020-01-01 00:00:00 <= X < 2022-01-01 00:00:00
PeriodFactory::fromYear(new DateTime('2020-07-01 13:05'))->toYearInclusive(new DateTime('2021-02-26 11:48:40'));

All of these periods simply have arbitrary precision. The precision and range inclusion only has meaning in the factory, and it's all about semantics. You could of course have some "metadata" in the period object to remember how it was produced, but that should not affect the way it works at all.

Maybe you're wondering about the boundary inclusion and exclusion here. It really boiles down to these two being perfectly identical (except in semantics).

  • Van 1 januari tot 1 februari
  • Van 1 januari t/m 31 januari

(I used our native language here because of the clear distinction between tot vs t/m)

@brendt
Copy link
Collaborator Author

brendt commented Mar 1, 2021

In that case arbitrary makes sense indeed. However I'm a little afraid it'll cause confusion, especially because the goal of this package is to only be able to work with periods that you know the exact precision of. (you can read https://stitcher.io/blog/comparing-dates for some background info)

@ameenross
Copy link

ameenross commented Mar 1, 2021

About that, I must say I disagree. To me, there's no difference between a period starting with 2021-01-01 with day precision, and a period starting with 2021-01-01 00:00 with minute precision. I am used to comparing DateTime objects directly and would not have it any other way. Anything other than that (pure mathematics) is semantics (e.g. "the period of January"), which I would keep separate as much as possible as you can see in my example. In fact, in my factory example you could also do this:

// Month of january: 2021-01-01 00:00:00 <= X < 2021-02-01 00:00:00
PeriodFactory::entireMonth(new DateTime('2021-01-13'));
PeriodFactory::entireMonth(2021, 'january');

// Entire year: 2021-01-01 00:00:00 <= X < 2022-01-01 00:00:00
PeriodFactory::entireYear(new DateTime('2021-03-01 10:19'));
PeriodFactory::entireYear(2021);

Again, that's all about semantics, but the eventual mathematics is no different.

@brendt
Copy link
Collaborator Author

brendt commented Mar 2, 2021

So the only reason this package exists is because I found it to be counter intuitive to think in that way about dates. Maybe https://github.com/thephpleague/period would be a better match if that reasoning doesn't work for you?

FYI I've been thinking so more about operations on periods with different boundaries. The easiest solution would be to disallow those altogether, just like periods with different precisions can't be used. I realise this might make the package less interesting to some, but there are great alternatives out there if you want more flexibility.

@ameenross
Copy link

ameenross commented Mar 2, 2021

So the only reason this package exists is because I found it to be counter intuitive to think in that way about dates.

But it's perfectly valid mathematically, is it not? And for users of the library it's mostly irrelevant how it works under the hood, as long as it does work.

Just one simple question that you couldn't answer in any other way (unless with convoluted workarounds) is "how many seconds does March contain". It would be $end->getTimestamp() - $start->getTimestamp(), where start is new DateTime('2021-03-01') and end is new DateTime('2021-04-01').

Maybe https://github.com/thephpleague/period would be a better match if that reasoning doesn't work for you?

I didn't even manage to find that library last time around. I'll have to look into it further, but having started out with spatie/period, I'm somewhat reluctant. Although I do like some things I also see some things that are off-putting to me. I'll stick with spatie for now.

The easiest solution would be to disallow those altogether

I agree, that makes the most sense to do. To be honest I think I would only implement [x₁, x₂) type intervals, then it wouldn't be a problem. Again, the "end inclusive" interpretation can be used in some factory method (or named constructor if you fancy that term) or methods. However, in the context of how this package currently works, disallowing "mixed usage" is safest (I suspect nobody would want to do that to begin with, unless they want to shoot themselves in the foot).

I would like to give another reason for using Precision::ARBITRARY. It's a common theme to see 23:59:59, so as to prevent using 00:00:00 or 24:00:00. Honestly, that is a mistake (actually I don't even see the benefit). One of the reasons that is a mistake is the following scenario:

$march = new stdclass;
$march->start = new DateTime('2021-03-01');
$march->end = new DateTime('2021-03-31 23:59:59');

$dt = new DateTime('2021-03-31 23:59:59.297398');
$march->start <= $dt && $dt <= $march->end; // false: $dt not part of march

I have actually had to fix bugs regarding this when PHP 7 came around with microsecond support in PHP DateTime. In fact, microsecond support broke a whole bunch of unit tests.

@brendt brendt mentioned this pull request Mar 11, 2021
@brendt
Copy link
Collaborator Author

brendt commented Mar 11, 2021

@ameenross I gave it some more thought and decided to be more flexible. Take a look at these test cases:

    public function boundariesForSubtract(): Generator
    {
        yield [
            'period' => '[2021-01-01,2021-02-01)',      //          [=========)
            'subtract' => '[2021-01-15,2021-02-01]',    //                  [=====]
            'result' => '[2021-01-01,2021-01-15)',      //          [=======)
        ];

        yield [
            'period' => '[2021-01-01,2021-02-01)',      //          [=========)
            'subtract' => '(2021-01-15,2021-02-01]',    //                  (=====]
            'result' => '[2021-01-01,2021-01-16)',      //          [========)
        ];

        yield [
            'period' => '[2021-01-01,2021-02-01]',      //          [=========]
            'subtract' => '(2021-01-15,2021-02-01]',    //                  (=====]
            'result' => '[2021-01-01,2021-01-15]',      //          [=======]
        ];

        yield [
            'period' => '[2021-01-01,2021-02-01]',      //          [=========]
            'subtract' => '[2021-01-15,2021-02-01]',    //                  [=====]
            'result' => '[2021-01-01,2021-01-14]',      //          [======]
        ];

        yield [
            'period' => '[2021-01-01,2021-02-01]',      //                  [=======]
            'subtract' => '[2021-01-01,2021-01-10]',    //          [=========]
            'result' => '[2021-01-11,2021-02-01]',      //                     [====]
        ];

        yield [
            'period' => '(2021-01-01,2021-02-01]',      //                  (=======]
            'subtract' => '[2021-01-01,2021-01-10]',    //          [=========]
            'result' => '(2021-01-10,2021-02-01]',      //                    (=====]
        ];

        yield [
            'period' => '(2021-01-01,2021-02-01]',      //                  (=======]
            'subtract' => '[2021-01-01,2021-01-10)',    //          [=========)
            'result' => '(2021-01-09,2021-02-01]',      //                   (======]
        ];

        yield [
            'period' => '[2021-01-01,2021-02-01]',      //                  [=======]
            'subtract' => '[2021-01-01,2021-01-10)',    //          [=========)
            'result' => '[2021-01-10,2021-02-01]',      //                    [=====]
        ];
    }

    public function boundariesForOverlap(): Generator
    {
        yield [
            'period' => '[2021-01-01,2021-02-01)',     //          [==================)
            'overlap' => '[2021-01-10,2021-01-15]',    //               [=========]
            'result' => '[2021-01-10,2021-01-16)',     //               [==========)
        ];

        yield [
            'period' => '[2021-01-01,2021-02-01)',     //          [==================)
            'overlap' => '[2021-01-10,2021-01-15)',    //               [=========)
            'result' => '[2021-01-10,2021-01-15)',     //               [=========)
        ];

        yield [
            'period' => '[2021-01-01,2021-02-01)',     //          [==================)
            'overlap' => '[2021-01-10,2021-02-15)',    //                      [=========)
            'result' => '[2021-01-10,2021-02-01)',     //                      [======)
        ];

        yield [
            'period' => '[2021-01-01,2021-02-01)',     //          [==================)
            'overlap' => '[2021-01-10,2021-02-15]',    //                      [=========]
            'result' => '[2021-01-10,2021-02-01)',     //                      [======)
        ];

        yield [
            'period' => '[2021-01-01,2021-02-01)',     //          [==================)
            'overlap' => '[2021-01-01,2021-01-15]',    //     [===========]
            'result' => '[2021-01-01,2021-01-16)',     //          [=======)
        ];

        yield [
            'period' => '[2021-01-01,2021-02-01)',     //          [==================)
            'overlap' => '[2021-01-01,2021-01-15)',    //     [===========)
            'result' => '[2021-01-01,2021-01-15)',     //          [======)
        ];

        yield [
            'period' => '(2021-01-01,2021-02-01)',     //          (==================)
            'overlap' => '[2021-01-10,2021-01-15]',    //                   [======]
            'result' => '(2021-01-09,2021-01-16)',     //                  (========)
        ];

        yield [
            'period' => '(2021-01-01,2021-02-01)',     //          (==================)
            'overlap' => '(2021-01-10,2021-01-15)',    //                   (======)
            'result' => '(2021-01-10,2021-01-15)',     //                   (======)
        ];
    }

@brendt
Copy link
Collaborator Author

brendt commented Mar 11, 2021

FYI I'm closing in to merging this PR. Final feedback is still welcome. We'll stick with configurable boundaries, with the default being both ends included, I realise this is not the way how some want to use the package, and there are already great alternatives out there if you think this package doesn't fit your needs. I strongly believe that flexible boundaries combined with precision is what differs this package from others, and — in my opinion — makes it easier to work with.

@brendt brendt marked this pull request as ready for review March 11, 2021 08:30
@brendt brendt merged commit d1b16dd into master Mar 17, 2021
@brendt brendt deleted the 2.0 branch March 17, 2021 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants