spatie / period

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

v2 #80

Closed brendt closed 3 years ago

brendt commented 3 years ago
brendt commented 3 years ago

@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 commented 3 years ago

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

brendt commented 3 years ago

TODO:

ameenross commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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.

brendt commented 3 years ago

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

ameenross commented 3 years ago

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 commented 3 years ago

Why ARBITRARY and not MICROSECONDS ?

ameenross commented 3 years ago

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).

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

brendt commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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.