Split ZoneOffset into UtcOffset and FixedOffsetTimeZone #125

ilya-g · 2021-06-04T08:14:51Z

This PR separates the concept of a time zone with a fixed UTC offset and from the concept of UTC offset itself.

Remaining task items:

serialization of UtcOffset
decide on UtcOffset normalization
~~make UtcOffset comparable~~
- ~~decide on the default ordering~~

Work postponed for a further PR:

documentation of new public API
Allow non-normalized fixed offset time zone, such as UTC+04 or Etc/UTC, to be represented by FixedOffsetTimeZone preserving its id: PR Normalize to FixedOffsetTimeZone in JVM and JS #130

core/build.gradle.kts

core/common/src/TimeZone.kt

ilya-g · 2021-06-08T14:43:37Z

core/common/src/serializers/TimeZoneSerializers.kt

        encoder.encodeString(value.id)
    }

 }
+
+public object UtcOffsetSerializer: KSerializer<UtcOffset> {


We could also provide an alternative serializer that stores offset as an integer primitive or a single component object.

core/jvm/src/TimeZoneJvm.kt

core/common/test/InstantTest.kt

core/common/test/TimeZoneTest.kt

core/darwin/src/Converters.kt

dkhalanskyjb · 2021-06-22T08:55:35Z

core/js/src/TimeZone.kt

@@ -28,12 +27,12 @@ public actual open class TimeZone internal constructor(internal val zoneId: Zone

    public actual companion object {
        public actual fun currentSystemDefault(): TimeZone = ZoneId.systemDefault().let(::TimeZone)
-        public actual val UTC: TimeZone = jtZoneOffset.UTC.let(::TimeZone)
+        public actual val UTC: FixedOffsetTimeZone = UtcOffset(jtZoneOffset.UTC).asTimeZone()

        public actual fun of(zoneId: String): TimeZone = try {
            val zone = ZoneId.of(zoneId)
            if (zone is jtZoneOffset) {


~~Didn't we want to do zone.rules.isFixedOffset here instead and normalize the fixed-offset time zones?~~ You said that this would be in the following PR. Keeping the message for the following note:

By the way, somewhat bad news: 310bp implements isFixedOffset as follows:

public boolean isFixedOffset() { return savingsInstantTransitions.length == 0; }

So, I think this means that there is no way to discern the inherently fixed-offset time zones from those that are simply yet to have their first transition.

Any examples of such time zones?

Not that I know of.

dkhalanskyjb · 2021-06-22T09:06:39Z

core/js/src/UtcOffset.kt

+
+    override fun hashCode(): Int = zoneOffset.hashCode().toInt()
+    override fun equals(other: Any?): Boolean = other is UtcOffset && this.zoneOffset == other.zoneOffset
+    override fun toString(): String = zoneOffset.toString()


Looking at the Native implementation, these methods are simple and seemingly don't gain anything by being delegated to ZoneOffset, so I don't see a reason to store that as opposed to just a plain number of seconds.

I'd keep ZoneOffset instance for interop with js-joda library.

js-joda caches ZoneOffset instances: https://github.com/js-joda/js-joda/blob/master/packages/core/src/ZoneOffset.js#L287, so I don't think constructing ZoneOffset on the fly in the converter functions could become a bottleneck.

Same goes for the Java implementation.

core/jvm/src/Converters.kt

dkhalanskyjb · 2021-06-22T09:16:58Z

core/jvm/src/TimeZoneJvm.kt


 public actual fun Instant.toLocalDateTime(timeZone: TimeZone): LocalDateTime = try {
    java.time.LocalDateTime.ofInstant(this.value, timeZone.zoneId).let(::LocalDateTime)
 } catch (e: DateTimeException) {
    throw DateTimeArithmeticException(e)
 }

+internal actual fun Instant.toLocalDateTime(offset: UtcOffset): LocalDateTime = try {
+    java.time.LocalDateTime.ofInstant(this.value, offset.zoneOffset).let(::LocalDateTime)


If we do decide not to store a ZoneOffset inside a UtcOffset, then LocalDateTime.ofEpochSecond would probably fit better here.

core/native/src/Instant.kt

core/native/src/TimeZone.kt

ilya-g · 2021-06-22T02:10:40Z

core/common/src/serializers/TimeZoneSerializers.kt

        val zone = TimeZone.of(decoder.decodeString())
-        if (zone is ZoneOffset) {
+        if (zone is FixedOffsetTimeZone) {
            return zone
        } else {
            throw SerializationException("Timezone identifier '$zone' does not correspond to a fixed-offset timezone")


May be a problem if a time zone is recognized as fixed-offset in one platform, but not as fixed-offset in other.

Extract some former native-only ZoneOffset tests into common UtcOffset tests.

On the contrary, two FixedOffsetTimeZones with the same offset can differ by id (this aspect is native-only for now).

Roundtripping zone names when converting FixedOffsetTimeZone to NSTimeZone and back is not guaranteed. Instead ensure that the returned time zone is a FixedOffsetTimeZone and has the same offset.

Override some methods in FixedOffsetTimeZone to provide a simpler implementation

dkhalanskyjb · 2021-07-05T07:06:03Z

core/common/src/UtcOffset.kt

+        public fun parse(offsetString: String): UtcOffset
+    }
+}
+public expect fun UtcOffset(hours: Int? = null, minutes: Int? = null, seconds: Int? = null): UtcOffset


This needs to be documented. The other new functions as well, but the rest of them are much more self-explanatory.

ilya-g force-pushed the utc-offset branch from c3df865 to 3da3dd6 Compare June 7, 2021 23:18

ilya-g marked this pull request as ready for review June 8, 2021 14:08

ilya-g requested a review from dkhalanskyjb June 8, 2021 14:08

ilya-g commented Jun 8, 2021

View reviewed changes

ilya-g force-pushed the utc-offset branch 2 times, most recently from dd35124 to 6e0f75c Compare June 21, 2021 15:05

dkhalanskyjb requested changes Jun 22, 2021

View reviewed changes

ilya-g commented Jun 22, 2021

View reviewed changes

ilya-g force-pushed the utc-offset branch 2 times, most recently from c01b720 to 88f8ca3 Compare July 2, 2021 15:04

ilya-g added 11 commits July 2, 2021 18:06

Split ZoneOffset into UtcOffset and FixedOffsetTimeZone classes

498bf5c

Extract some former native-only ZoneOffset tests into common UtcOffset tests.

Make UtcOffsets with the same offset value indistinguishable

53f0224

On the contrary, two FixedOffsetTimeZones with the same offset can differ by id (this aspect is native-only for now).

Soften assertion in zoneOffsetToNSTimeZone conversion test

8cd4ab7

Roundtripping zone names when converting FixedOffsetTimeZone to NSTimeZone and back is not guaranteed. Instead ensure that the returned time zone is a FixedOffsetTimeZone and has the same offset.

Refactor native TimeZone to avoid member-extension internal functions

f7d5e45

Override some methods in FixedOffsetTimeZone to provide a simpler implementation

Make TimeZone.UTC return FixedOffsetTimeZone (statically)

c4d4d3a

UtcOffset serialization (as a string primitive)

130a0ec

Ensure UtcOffset.parse throws DateTimeFormatException

8f4848d

Make Instant.toLocalDateTime(UtcOffset) internal

318d5c3

Public parameters of UtcOffset type: rename to just 'offset'

90c0864

Introduce UtcOffset.ZERO constant

cadd19f

UtcOffset construction from components

a35ae66

ilya-g force-pushed the utc-offset branch from 88f8ca3 to a35ae66 Compare July 2, 2021 15:06

ilya-g added this to the 0.3.0 milestone Jul 2, 2021

ilya-g mentioned this pull request Jul 2, 2021

Provide construction and/or parsing for ZoneOffset #91

Closed

ilya-g merged commit a35ae66 into master Jul 2, 2021

dkhalanskyjb reviewed Jul 5, 2021

View reviewed changes

ilya-g deleted the utc-offset branch September 29, 2021 15:22

Split ZoneOffset into UtcOffset and FixedOffsetTimeZone #125

Split ZoneOffset into UtcOffset and FixedOffsetTimeZone #125

Uh oh!

Conversation

ilya-g commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ilya-g commented Jun 4, 2021 •

edited

Loading