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

WIP: Added ZonedDateTime #175

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wkornewald
Copy link
Contributor

@wkornewald wkornewald commented Dec 29, 2021

Initial sketch of

  • ZonedDateTime: common base class
  • RegionDateTime: works for past and future dates and mimics how humans see dates
  • OffsetDateTime: how many official specifications represent past/present dates

This is still missing a RegionTimeZone to represent time zones based on a region id instead of just an offset. Not implemented yet:

  • parsing
  • serialization
  • support for adding/subtracting durations
  • calculating the difference between two ZonedDateTimes

Also see discussion in #163

Feedback welcome.

This was referenced Dec 29, 2021
@dkhalanskyjb
Copy link
Collaborator

  • As defined, ZonedDateTime is just a collection of functions that are very easy to implement and doesn't offer much over just storing LocalDateTime and TimeZone separately. If something is this easy to implement and doesn't meaningfully interact with the other entities in the library, there's little point in adding it to the base library.
  • It's unclear how OffsetDateTime and RegionDateTime could be useful. When is it important to know what exactly does the ZonedDateTime store? If you need it for checking whether the time zone is fixed-offset, why not check the time zones themselves?
  • OffsetDateTime is probably not the best name for this, as it stores a FixedOffsetTimeZone and not an offset.
  • As you yourself note, instant is ill-defined, though for more problematic reasons: LocalDateTime to TimeZone conversion is ambiguous, which ZonedDateTime hides.

@wkornewald
Copy link
Contributor Author

wkornewald commented Jan 10, 2022

  • As defined, ZonedDateTime is just a collection of functions that are very easy to implement and doesn't offer much over just storing LocalDateTime and TimeZone separately. If something is this easy to implement and doesn't meaningfully interact with the other entities in the library, there's little point in adding it to the base library.
  1. We can serialize/deserialize, format/parse, sort and compare dates very easily. It's impossible or too difficult and error prone if we store those separately.
  2. This is a draft/WIP and I wanted to add support for adding/subtracting durations and other operations which map to how humans think about dates.
* It's unclear how `OffsetDateTime` and `RegionDateTime` could be useful. When is it important to know what exactly does the `ZonedDateTime` store? If you need it for checking whether the time zone is fixed-offset, why not check the time zones themselves?

When you want to use static types to enforce minimum requirements. We might want to ensure we store non-ambiguous RegionDateTime in the backend or maybe some OpenAPI endpoint only supports OffsetDateTime. Enforcing this with static types prevents mistakes and non-trivial bugs.

* `OffsetDateTime` is probably not the best name for this, as it stores a `FixedOffsetTimeZone` and not an offset.

That's just the name other APIs (e.g. java.time) use, but we can rename it to FixedOffsetDateTime. Though, IMHO OffsetTimeZone would've been a nicer name without the Fixed prefix and it's pretty clear that the offset is fixed.

* As you yourself note, `instant` is ill-defined, though for more problematic reasons: `LocalDateTime` to `TimeZone` conversion is ambiguous, which `ZonedDateTime` hides.
  1. It's not ambiguous with OffsetDateTime. It's also not ambiguous with RegionDateTime when adding the optional offset (see the TODO) like java.time and the new JavaScript Temporal API do. Also it would be the correct building block for a calendar app where you often have to do date arithmetic (for repeating events).
  2. It's the way humans think about time (and how a calendar app would work). So even if it's ambiguous, the ambiguity is just an edge case which can be resolved with RegionDateTime. In general it works well enough for humans. Also, this functionality is important and complex enough that we need an API for it.

@cromefire
Copy link

  1. It's also not ambiguous with RegionDateTime when adding the optional offset

In which case it would just be a OffsetDateTime right? And all the calendar usage traits would be gone.

core/common/src/ZonedDateTime.kt Show resolved Hide resolved
public constructor(instant: Instant, timeZone: TimeZone) :
this(instant.toLocalDateTime(timeZone), timeZone)

// TODO: Should RegionTimeZone.toString() print with surrounding `[]`?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any standards for this?

Copy link
Contributor Author

@wkornewald wkornewald Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostgreSQL, Elasticsearch, java.time, JS Temporal and others seem to have settled on this full format: 2021-03-28T03:16:20+02:00[Europe/Berlin]

Though, maybe RegionTimeZone itself should just be Europe/Berlin and the surrounding [] would be added by RegionDateTime. Most libs will parse the region without the [].

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I'd settle with the established format

public constructor(instant: Instant, timeZone: FixedOffsetTimeZone) :
this(instant.toLocalDateTime(timeZone), timeZone)

override fun toString(): String = "$localDateTime$timeZone"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be in RFC3339 as that's the common serialization and representation everywhere.

@wkornewald
Copy link
Contributor Author

wkornewald commented Jan 13, 2022

  1. It's also not ambiguous with RegionDateTime when adding the optional offset

In which case it would just be a OffsetDateTime right? And all the calendar usage traits would be gone.

No, the UTC offset is used for disambiguation. The region is still needed. We need to handle these cases:

  • A country decides to get rid of DST or introduce DST or change the DST offset (most of Europe is planning the first one, so your calendar entries better don’t break).
  • A country moves its whole time zone offset.
  • You want to refer to a moment within a winter<->summer time change.

For calendar applications the region would always take precedence and the offset would be used to disambiguate only. Example: Your wedding event in Italy in 2024 at 14:00 must keep its local time at 14:00 even if Europe gets rid of DST handling in the meantime. So, the offset must be ignored unless there's an ambiguity and even then it should only used to handle DST offsets and never be applied literally.

See also how others are doing it:

The only open question is whether we should support a DST flag in addition to UTC based offsets. If a country moves its time zone offset and this overlaps with a DST switch then a UTC offset alone can't correctly disambiguate the hour during a DST change. Only a DST flag could handle that edge case. Maybe that's too much of an edge case though and it may not even be clear what the user's original intent was.

@cromefire
Copy link

In that case I'd stick to the TC39 proposal, that looks quite good (there's also usually with at lot of peoples checking it).

@kkocel
Copy link

kkocel commented Nov 23, 2022

I port some JVM-related code and this exact class is needed. Hope that it gets merged...

@dkhalanskyjb
Copy link
Collaborator

@kkocel most probably the Instant arithmetic, which already exists, will suit you: https://github.com/Kotlin/kotlinx-datetime#instant-arithmetic

@kkocel
Copy link

kkocel commented Nov 23, 2022

@dkhalanskyjb actually I need to have a class that can be deserialized from the java.time.ZonedDateTime that comes from JVM backend

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

Successfully merging this pull request may close these issues.

4 participants