-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Comments
My preference would be an implicit 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 ( 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 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 |
@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. |
I like current behavior. We could maybe give more strong names for params in
|
Unfortunately we still will need to decide how to serialize this object to string |
After further thinking and discussions, I think we'll just accept & document that |
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:
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()
andgetEnd()
are implicitly exclusive, and that you need to useofClosed()
/getEndInclusive()
to make them inclusive.Therefore I'm thinking of making everything explicit, which leaves no doubt but is a little ugly:
Notes:
ofEndExclusive()
, but at least it's clear on the intent, rather thanofHalfOpen()
for example; suggestions welcomegetStart()
be calledgetStartInclusive()
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:
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?
The text was updated successfully, but these errors were encountered: