-
Notifications
You must be signed in to change notification settings - Fork 103
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
Add a ZonedDateTime type #163
Comments
Well for everything else, just use |
True. However, there's a difference between introducing an
I'm not sure what "fixing an offset" means, but to get an fun ldtAndOffsetToInstant(pair: Pair<LocalDateTime, UtcOffset>): Instant {
val (ldt, offset) = pair
return ldt.toInstant(offset)
}
At the moment, we don't provide means to handle this. |
The helper functions definite the semantics of the class, so I don't see a difference here.
Convert from a TimeZone (dynamic-ish offset) to a fixed offset.
I'd define this as a goal though. |
The difference is that there are fundamental classes that define the problem area—we've chosen
Timezones have different offsets at different times. Daylight saving time is an example.
Sure, we are planning an API for parsing and formatting, I said this several times in the other thread. |
Sorry, noticed too late, moving discussion here.
Wow. This looks more like a bug. Try going from summer to winter time: val d2 = ZonedDateTime.parse("2021-10-31T02:16:20+02:00[Europe/Berlin]")
// => 2021-10-31T02:16:20+02:00[Europe/Berlin]
d2.plusDays(1)
// => 2021-11-01T02:16:20+01:00[Europe/Berlin]
d2.plusHours(24)
// => 2021-11-01T01:16:20+01:00[Europe/Berlin]
ZonedDateTime.ofInstant(d2.toInstant().plus(1, ChronoUnit.DAYS), ZoneId.of("Europe/Berlin"))
// => 2021-11-01T01:16:20+01:00[Europe/Berlin]
Your first example just happened to auto-correct the invalid offset that is disallowed by the zone - and this is documented as an edge case. It just recomputed to a different offset, but that doesn't mean it stores an Instant.
You're already suggesting to use No
No my argument is: Humans often think in terms of LocalDateTime+TimeZone, so we need a solution for that. And ideally it also has a bug-free
Actually it is true, but you seem to have found a bug (or kind-of bad design). Let's imagine there was no bug. 😅 |
Well I guess in that would make it something in between. It's not a primitive/fundamental type, but it'll probably be a bit more than just a helper class I guess? Depends on if you still classify it as such. I've never argued for it to be a fundamentally brand new type, most functions would probably just be one liner and the biggest benefits would be documentation and clearer (and easier) code. Most things can already be done today if you where willing to handle both separately, but with the compound type it'll be a lot easier.
Yes, that's why you can only convert it to an offset for a single point of time and even that may not be unambiguous.
Yeah I'd probably add some default support though, if there is a common format being used for this. |
This behavior is documented fairly well, and, I think, it's the only consistent way to implement this given the constraints of
Nope, it's not just as good:
It's only an edge case if you consider it an edge case if two machines have different timezone databases. If you attempt to send a
Sure. Do we agree that
Imagining it requires implementing a whole new data structure, unfortunately, as this behavior is inherent in
For each moment in time, there's an unambiguous offset for each |
Well yes for that case, the same is not true for
It was pretty fine last time I used it. We can probably get something more lightweight going, but the basic principles seemed fine.
Yes that is the point. Because it should not represent a moment in time. |
It could be fine if you used it as
I was answering these words:
Nope, for a single point in time (that is, |
Not quite. Future zone changes have different results depending on whether you pick LocalDateTime or Instant because the meaning of a future ZonedDateTime changes. In the LocalDateTime case the local time is stable and the serialization is stable (because you exactly reproduce the formatted string). In the Instant case the UTC time is stable, but the serialization is unstable and even incorrect from a typical end-user point of view because the formatted time suddenly changes, but users want only the offset to change. Human-friendly calendar-like computations ( Though, maybe java.time.ZonedDateTime doesn't treat all of this as well as I'd like to have, so maybe we should shift the discussion a little bit.
When adding durations to LocalDateTime+TimeZone you also can't treat them as disjoint data. The dependency stems from what you want to do with the data.
I'd consider your example a much more extreme edge case than the actual intention of LocalDateTime+TimeZone: Correcting zone changes for events in the future. That's a much more likely issue.
Yes we surely agree that Java's implementation is not ideal. In my mind the ZonedDateTime should better match how humans think.
For |
That is the central point I think. This type should be used for user facing stuff and therefore work how a normal person would expect it to. |
Just FYI, in the JavaScript ecosystem the proposed Temporal API also could provide some interesting insights: https://tc39.es/proposal-temporal/docs/ambiguity.html |
I've created an initial PR #175 with a sketch of what the API could look like. @dkhalanskyjb @cromefire Would be nice to have your feedback and possibly counter-PRs or PRs on top of my PR. :) |
Here is a use case that is easy with a Java Given a For example, given zone With Java
outputs:
where we can clearly see 1:30 am is repeated twice with two different offsets. I can solve the problem easily on the JVM using another feature of
which outputs as expected:
so we can easily solve the original problem like this:
The closest equivalent I've come up with for kotlinx-datetime would be something like this:
however this makes all sorts of incorrect assumptions that a) While practically that implementation will likely work for most cases, its uncomfortable to say the least. |
Not sure what you mean, we also document this behavior: kotlinx-datetime/core/common/src/TimeZone.kt Lines 158 to 159 in 5a920ea
I can think of a way to replace the assumption that the overlap period is always 1 hour by an assumption that there's at most one transition in a given day:
However, it's true that this is also not completely future-proof. We are planning to solve this issue by adding a parameter to |
(source: My team has spent multiple person-years trying to eradicate date/time bugs in Google's codebase.) FWIW, the abseil library for C++ takes a stand against the existence of this type, and I agree with them. I think such a type is ambiguous, dangerous, and unnecessary. It's dangerous because it becomes easy to write natural-looking code, where no time zone is seen, that asks a question like "what is one day after x?" in two subtly different ways. Since the data type has a zone tucked away, it's capable of answering both versions of the question, and to most users the difference won't be clear. The cleaner approach (that kotlinx has now!) is for all code to be unambiguously either about physical time or about civil time, not a mixture of the two. A time zone is used only as a (nearly-invertible) function for going back and forth between the two. Note, the alternative is not necessarily Pair. Most often, the zone is stored at some broader scope. This forces you to think about which of Instant or LocalDateTime is the one you really want to store, since that broader time zone setting might change. But you'd really better do that anyway in case the zone data changes. |
Well right now it only has physical time, civil time is only DIY, you have to implement a means of storage and helpers on your own, because sometimes the timezone is stored in a broader context (in which case, yes just use |
I led the design work for JavaScript's new Temporal.ZonedDateTime type, and I stumbled upon this issue while doing research for the upcoming IETF standardization of the If you're interested, feel free to read through tc39/proposal-temporal#700 which is the original design doc for the If you do go down the path of ZonedDateTime, I'll share some advice that would have been helpful for us when we started designing this API 2 years ago:
Anyway, I hope you find the above useful, and I wish you good luck in designing a really fun and challenging API! If there's any questions you have, feel free to reach out. I'm happy to help. |
Would potentially good to be compatible with as much stuff as possible and you definitely seem to have put in more work that we did discussing it on like a few days. |
Hi @justingrant! Thanks for reaching out, and congratulations on landing this controversial functionality! I'm a bit confused by your message though, because either I'm misunderstanding it greatly, or you're not aware that we already have this functionality, just with a different API. Let's break this down point by point.
|
Oh, this is an interesting solution! Sorry for missing this, my knowledge of Kotlin is limited. It's cool to see how different people have come up with different solutions to the problem of developers ignoring DST. Your solution (every method that would have required a ZDT instead requires both an Instant and a TimeZone) seems reasonable to me, as does omitting math from
Our solution to this was for the default behavior to match what's specified in RFC 5545 and what was used by the pre-Temporal JS specification. So the default behavior was kind of a no-brainer because we just followed existing practice. Here's the text from RFC 5545 Section 3.3.5:
And here's the text from the pre-Temporal ECMAScript language spec:
Temporal also provide a
Was that the info you were looking for?
Yeah, intermediate values are confusing. The case you linked to seems like the easy version of this confusing behavior, because the caller is making separate method calls, so they can inspect the results to understand more about what's happening with the intermediate values. IMO the more confusing case is adding a Note that we don't support the DST An even harder case we had no explicit guidance from any existing standard was when calculating differences (
Oh, I wasn't aware that this behavior was also specified in ISO 8601. Do you have a citation for the version and section number? The newer ISO 8601 specs (esp. the -1 and -2 supplements) are quite, er, voluminous so it's not surprising that we missed that. |
@dkhalanskyjb The In other words, Instant is only useful when working with physical time while ZonedDateTime could represent time in the way humans think. They are not the same and while the Instant arithmetic API might work 90% of the time it's still overall the wrong approach which leads to incorrect future date handling. Apart from the correctness issue it's very inconvenient to always work with two separate values. You even need three values to represent the same level of detail as ZonedDateTime. This separation also makes parsing/serialization more difficult unnecessarily. How do I work with my JSON data which contains zone information? Do I have to create an ad-hoc data class and serializer which is a mediocre version of ZonedDateTime? Basically the whole world uses a combined string/field that includes the datetime and zone. Why would anyone want to deal with two (or three) separate values all the time instead of having just one? |
We already have that behavior by default as well. I was interested in the
ISO-8601 provides an unambiguous definition of a difference (called "duration" there). See 3.1.1.8 for the definition of the duration, 3.1.2 for the specific definitions of time units used in the document, and 5.5.2 for the definition of the composite duration. The version I'm looking at is ISO 8601-1, edition 2019-02. This is also the answer for
I don't think 8601-1 is a supplement. Looking at the document, it says "part 1: basic rules". What is the actual ISO 8601 then? |
@dkhalanskyjb AFAIK, ISO doesn't specify how to calculate a difference between two ZonedDateTime instances, primarily because it doesn't consider such entity as ZonedDateTime at all. |
The duration is specified for |
Good question. We were mostly following the lead of Abseil Time (the C++ library) which has fine-grained control for operations like this. We didn't have specific use cases in mind-- but we had heard from a few users that they wanted customized control.
Sorry, it's been two years since I read the ISO 8601 specs in detail. I believe you're correct.
OK I'll take a look. Thanks for the pointer.
If you're working with timezone-aware data, you need to choose whether the data is Instant + TimeZone or (using Java/Kotlin terms here) LocalDateTime + TimeZone. There is no perfect choice. Some use cases work better with Instant + TimeZone, while others work better with LocalDateTime + TimeZone. We decided that Instant + TimeZone was the right combo because it's unambiguous. In order to make LocalDateTime + TimeZone unambiguous, we'd have needed to store the offset too. This could be done, of course, but it would have added to the complexity of the type. To be clear, this was a judgement call. We could have chosen the other option, but we didn't. |
If you want correctness you really need both the offset and zone ID. The added "complexity" is very minor here and I hope the solution for kotlinx.datetime won't be "let's make it simpler even if it means it's only 90% correct". The current solution is just unacceptable for calendar apps and similar use-cases. |
At this point, this discussion could benefit from being more structured. So, we opened a topic in GitHub Discussions: #237 Please chime in! The format of GitHub Discussions is new for us, so we don't have a set of best practices in place, but initially, let's try the following:
|
BTW, draft-ietf-sedate-datetime-extended is nearing completion, which would extend RFC3339 with time zone information which would enable compatible exchange of zoned date times. This is what JS Temporal also implements and I hope it'll spread to other date time frameworks as well (like NodaTime for example). |
Any update ? |
Add a type that aligns with the normal calendar and combines TimeZone information with a date.
The text was updated successfully, but these errors were encountered: