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

Implement the same ranges for all platforms #453

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

dkhalanskyjb
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb commented Oct 31, 2024

Fixes #432

core/common/src/LocalDate.kt Outdated Show resolved Hide resolved
core/common/src/LocalDate.kt Outdated Show resolved Hide resolved
@dkhalanskyjb dkhalanskyjb force-pushed the uniform-ranges branch 3 times, most recently from b7c293c to ae4373e Compare November 5, 2024 12:52
@dkhalanskyjb dkhalanskyjb marked this pull request as ready for review November 5, 2024 12:52
* @see LocalDate.fromEpochDays
* @sample kotlinx.datetime.test.samples.LocalDateSamples.toEpochDays
*/
public fun toEpochDays(): Int
public fun toEpochDays(): Long
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered retaining source and/or binary compatibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We didn't find a way to choose an overload based on its return type, so we can't restore source compatibility. Also, on Native, the binary compatibility can't be restored, as two signatures with different return types cause a compilation error.

Restored the binary compatibility for the JVM.

core/common/src/LocalDate.kt Outdated Show resolved Hide resolved
core/common/src/DateTimePeriod.kt Outdated Show resolved Hide resolved
core/commonJs/src/internal/Platform.kt Outdated Show resolved Hide resolved
core/commonJs/src/internal/Platform.kt Outdated Show resolved Hide resolved
core/js/src/PlatformSpecifics.kt Show resolved Hide resolved
core/commonJs/src/internal/Platform.kt Show resolved Hide resolved
core/commonJs/src/internal/Platform.kt Outdated Show resolved Hide resolved
core/commonJs/src/internal/Platform.kt Outdated Show resolved Hide resolved
core/commonJs/src/internal/Platform.kt Outdated Show resolved Hide resolved
Comment on lines +87 to +89
zones[components[0]]?.let { rules ->
zones[components[1]] = rules
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the order of the two time zones specified? I mean is it possible that the second time zone has rules but the first doesn't?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://momentjs.com/timezone/docs/#/data-formats/link-format/ doesn't say anything either way, so the js-joda implementation is the best guidance we have on this, and they do assume this order.

core/commonKotlin/src/TimeZone.kt Outdated Show resolved Hide resolved
core/wasmJs/src/PlatformSpecifics.kt Outdated Show resolved Hide resolved
core/commonKotlin/src/Instant.kt Outdated Show resolved Hide resolved
core/commonKotlin/src/Instant.kt Outdated Show resolved Hide resolved
} catch (e: IllegalArgumentException) {
throw DateTimeArithmeticException("Boundaries of LocalDate exceeded when adding a value", e)
}
public actual fun LocalDate.plus(unit: DateTimeUnit.DateBased): LocalDate = plus(1, unit)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can the implementation be moved to the common source set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so. The JVM overload is in @file:JvmName("LocalDateJvmKt"), and moving it to LocalDateKt would be a breaking change. We could keep a LowPriorityInOverloadResolution DeprecationLevel.HIDDEN copy in LocalDateJvmKt, but then what's the point in moving it to common code? It's deprecated anyway.

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.

Make admissible ranges the same on all platforms
3 participants