brick / date-time

Date and time library for PHP
MIT License
337 stars 30 forks source link

LocalDateRange / endExclusive #70

Closed BenMorel closed 1 year ago

BenMorel commented 1 year ago

This https://github.com/brick/date-time/issues/45#issuecomment-1107905643 made be rethink of whether the current behaviour of LocalDateRange (start inclusive, end inclusive) is the correct one.

It was correct for my use case when I designed this class, but I realize that it may not be the same for everyone, and that it may depend on the use case. For consistency with other classes such as Interval, we could say that it should be exclusive of the end, but at the same time, I see a range of dates as a different beast from a range of date-times: in a calendar, if you say "from 2023-01-03 to 2023-01-10", I'd expect the 10th to be part of the range.

So I checked what's been done in threeten-extra in Java, and they actually offer both ways:

static LocalDateRange of(LocalDate startInclusive, LocalDate endExclusive);
static LocalDateRange ofClosed(LocalDate startInclusive, LocalDate endInclusive);

LocalDate getStart();
LocalDate getEnd(); // exclusive
LocalDate getEndInclusive();

I like the configurability of this behaviour, as it covers pretty much all use cases. Internally, we can choose either, as it doesn't matter to the outside world.

On the other hand, I'm personally not fond of the fact that of() and getEnd() are implicitly exclusive, and that you need to use ofClosed() / getEndInclusive() to make them inclusive.

Therefore I'm thinking of making everything explicit, which leaves no doubt but is a little ugly:

static function ofEndExclusive(LocalDate startInclusive, LocalDate endExclusive): LocalDateRange;
static function ofEndInclusive(LocalDate startInclusive, LocalDate endInclusive): LocalDateRange;

function getStart(): LocalDate;
function getEndExclusive(): LocalDate;
function getEndInclusive(): LocalDate;

Notes:


Alternatively, just copying the public API of threeten-extra as-is has a couple of advantages:

I'm still a bit worried by people misusing it by believing it's end-inclusive by default, but the parameter names can help I guess. Final thought: changing the default behaviour of of() / getEnd() is a quite heavy BC break, for which we can't trigger a deprecation notice.

What do you think, @antonkomarev, @solodkiy, @gnutix, @tigitz, @joshdifabio?

tigitz commented 1 year ago

My preference would be an implicit endInclusive by default and keeping the of method. As it is right now.

It's how business people express themselves around me when they talk about date and time range for subscriptions, delays etc.

So my brain would feel at ease mapping the default business use case with what seems to be the default way (of method) to express those concepts in the lib.

I think it's understandable for a library consumer to require a little bit of knowledge about how things work implicitly to properly use it. I mean I would prefer using of and keeping in mind that's endInclusive instead of writing ofEndInclusive everytime.

However it should be documented explicitly, internally parameters and properties should reflect this inclusiveness behavior. It should require just a single click on the method to instantly understand the behavior.

An ofEndExclusive could be added for people that need to express their range with exclusiveness. It would use the of method internally by subtracting 1 day to the end date.

BenMorel commented 1 year ago

@tigitz So basically what threeten-extra does, but in reverse :slightly_smiling_face:

tigitz commented 1 year ago

@tigitz So basically what threeten-extra does, but in reverse :slightly_smiling_face:

If you compare to the Java implementation then yes indeed 🙂

However, as a side note, I didn't consider at all what Java decided to do in my reasoning.

solodkiy commented 1 year ago

I like current behavior. We could maybe give more strong names for params in of() method, maybe we even should migrate to new ofEndInclusive method, but I personally don't think that we need some extra methods like ofEndExclusive.

ofEndExclusive is just ofEndInclusive + 1 day? I think concept of exclusion just does not have any sense for units that size (1 day).

solodkiy commented 1 year ago

I like the configurability of this behaviour, as it covers pretty much all use cases. Internally, we can choose either, as it doesn't matter to the outside world.

Unfortunately we still will need to decide how to serialize this object to string

BenMorel commented 1 year ago

After further thinking and discussions, I think we'll just accept & document that YearMonthRange and LocalDateRange are end inclusive, as that's how we usually think about such ranges.