Skip to content

Implement Instant.truncatedTo(unit: DateTimeUnit) #266

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

Closed

Conversation

FunnySaltyFish
Copy link

This PR mainly implement Instant.truncatedTo(unit: DateTimeUnit) for JVM/JS/Native to solves issue #223
The implementation for JVM and JS use their method, respectively, and the native one is implemented referring to Java's implementation.

And some extra changes were also included while implementing:

  • include date.h and tz.h to make it easier to run and compile
  • remove unnecessary @Suppress("NOTHING_TO_INLINE") in common/math.kt

@FunnySaltyFish FunnySaltyFish changed the title Feature instant truncated to Implement Instant.truncatedTo(unit: DateTimeUnit) Apr 13, 2023
@dkhalanskyjb
Copy link
Collaborator

Hi! Generally, it's not a good idea to implement something without first discussing it, because there's the risk of wasting effort.

  • I don't understand the inclusion of date.h and tz.h. Everything compiles and runs just fine as is.
  • The truncatedTo function does not recognize time zones, making it quite situationally useful. It would probably work just fine for seconds and minutes, but for hours, it would break for timezones whose offset differs from UTC by a non-whole number of hours. For days, it breaks for any non-UTC time zone.
  • Even if truncatedTo did recognize time zones, truncating to a DateTimeUnit.DAY this way would still be incorrect, as not every day is exactly 24 hours, due to DST transitions.
  • The functions you provide accept an arbitrary DateTimeUnit, but will in fact only work for time-based units and the DateTimeUnit.DAY, throwing on everything else (which isn't even mentioned in the docs). With the DateTimeUnit.DAY implementation being incorrect, it would make more sense to only accept DateTimeUnit.TimeBased.
  • Except it wouldn't even work properly for an arbitrary DateTimeUnit.TimeBased. How about the DateTimeUnit of seven seconds, for example? This would produce quite arbitrary results that can't be predicted by just reading the documentation.

Given all these issues, this clearly needs to be carefully thought out before it can be implemented in a library. To elaborate, I think this should be a part of the more general temporal adjustment API: functions like "next Wednesday after the given date" or "last time it was 00:00 on the clock" are quite similar and need to be designed in tandem. Maybe even not as part of the Instant API but with something like #237.

@FunnySaltyFish
Copy link
Author

Thanks for your quick and detailed response. It is my fault that dose not discuss before writing.
Here are some responses:

I don't understand the inclusion of date.h and tz.h. Everything compiles and runs just fine as is.

Actually, without adding them, I cannot even compile the library to Native (in my case, Windowsx86_64). IDEA will told me date/date.h is not found. Should I do something before compiling?

but for hours, it would break for timezones whose offset differs from UTC by a non-whole number of hours; For days, it breaks for any non-UTC time zone.

Yes, the implementation is basically following java.time.Instant and for your situations, it truly failed.

val instant2 = Instant.parse("2021-02-03T01:22:33.123456789+01:30:00")
assertTruncatedTo(instant2, DateTimeUnit.HOUR, Instant.parse("2021-02-03T11:00:00+01:30:00")) // failed
assertTruncatedTo(instant2, DateTimeUnit.DAY, Instant.parse("2021-02-03T00:00:00+01:30:00"))    // failed

...How about the DateTimeUnit of seven seconds?

Yes, in JVM and JS, it will be regarded as 1 second, but in Native, the behavior is unpredictable.


In the end, thank you for making me more aware of the issues I need to consider when writing a date-time library. And I do think methods like Instant.truncatedTo is helpful for developers. Look forward to your (and contributors') future work.

@dkhalanskyjb
Copy link
Collaborator

I cannot even compile the library

Please follow the instructions at https://github.com/Kotlin/kotlinx-datetime#building

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.

2 participants