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

LocalDateRange / endExclusive #70

Closed
BenMorel opened this issue Jun 25, 2023 · 6 comments
Closed

LocalDateRange / endExclusive #70

BenMorel opened this issue Jun 25, 2023 · 6 comments

Comments

@BenMorel
Copy link
Member

BenMorel commented Jun 25, 2023

This #45 (comment) 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:

  • I don't particularly like the naming ofEndExclusive(), but at least it's clear on the intent, rather than ofHalfOpen() for example; suggestions welcome
  • If we go this route, should getStart() be called getStartInclusive() for consistency? It's going a bit far IMO, as there are no classes that offer a start exclusive (I don't think there are many use cases for this)

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

  • people have already invested a lot of thinking into these APIs (like I was reminded in Resolve API inconsistencies regarding scalars versus objects #10 (comment) nearly 5 years ago), so maybe we should just accept that the decisions they took are the best compromise, and follow suite (this is part of a larger decision that is yet to be taken in this projet)
  • people coming from a Java background won't have to learn a new API
  • If we consider that other ranges such as Interval (and possibly incoming LocalDateTimeRange and ZonedDateTimeRange) are all exclusive of the end, if could make sense to be exclusive by default, and that the inclusive ones have to be explicitly suffixed.

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
Copy link
Contributor

tigitz commented Jun 25, 2023

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
Copy link
Member Author

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

@tigitz
Copy link
Contributor

tigitz commented Jun 25, 2023

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

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
Copy link
Contributor

solodkiy commented Jul 4, 2023

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
Copy link
Contributor

solodkiy commented Jul 4, 2023

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
Copy link
Member Author

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.

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

No branches or pull requests

3 participants