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

Feature Request: truncateTo for LocalTime #225

Closed
Stephen-Hamilton-C opened this issue Aug 31, 2022 · 6 comments
Closed

Feature Request: truncateTo for LocalTime #225

Stephen-Hamilton-C opened this issue Aug 31, 2022 · 6 comments
Labels
formatters Related to parsing and formatting

Comments

@Stephen-Hamilton-C
Copy link

When I was learning Kotlin, I made a JVM application that utilized the java.time.LocalTime class a lot, and it was convenient to use the truncateTo() method to only have hours and minutes. I'm now rewriting the application in Kotlin Native to get even more experience with the language, and I'm learning that to shave off seconds, I need to have an extra variable to truncate a LocalTime:

/**
 * The current system time, with seconds and nanoseconds
 */
private val untrimmedNow: LocalTime = Clock.System.now()
        .toLocalDateTime(TimeZone.currentSystemDefault()).time

/**
 * The current system time without seconds.
 */
val NOW: LocalTime = LocalTime(untrimmedNow.hour, untrimmedNow.minute)

While this works, I'd much rather only have to add one more line, something like .truncateTo(DateTimeUnit.SECOND).

@dkhalanskyjb
Copy link
Collaborator

Yes, we received similar requests already and are interested in adding this feature once we understand it better. Could you please describe why you need this?

@Stephen-Hamilton-C
Copy link
Author

The program has to track how long you've been "working" or "on break", and in this context, seconds and nanoseconds are negligible, and leaving only hours and minutes makes it trivial to format the time to the user (only have to do .toString() rather than formatting)

If you'd like the full context, you can find the source code here

@dkhalanskyjb
Copy link
Collaborator

I've taken a look at your code, and I think the whole approach is buggy in the presence of DST transitions. You use LocalTime to represent both moments of time and the difference between moments, but that is neither, and this does lead to bugs.

For example, semantically, core.TimeEntries#difference takes two moments and returns the duration of time between them. This is implemented in your code as arithmetic on LocalTime, but if, say, the clocks were moved from 15:00 to 16:00 that day, the difference between 14:59 and 16:01 should be just two minutes, not an hour and two minutes.

Another, more serious example is the behavior of the clockOut function if the clocks were moved backwards. Let's say I check in at 12:05, then, at 13:00, the clocks are moved back an hour, and a few minutes later, I clock out, at 12:01. The program will crash.

So, I suggest rethinking the whole approach:

  • Moments of time are represented as Instant values.
  • Duration of time between moments is either DateTimePeriod or Duration, depending on your needs. By the way, there's a convenient Instant.minus(other: Instant): Duration function.
  • LocalTime should only be used to display the time to the end user, it's too easy to make mistakes when trying to use this data structure in more elaborate scenarios. As a rule of thumb, if you do any arithmetic on its components, the code will break with DST transitions.

Still, the issue stands: it seems like you do want to disregard the sub-minute components when formatting. However, now it seems like your use case also calls for this functionality in Duration, not just LocalTime. @ilya-g, it looks like we have a request here for rounding Duration to a specified DurationUnit.

@dkhalanskyjb dkhalanskyjb added the formatters Related to parsing and formatting label Sep 27, 2022
@demidenko
Copy link

demidenko commented Aug 29, 2023

Yes, we received similar requests already and are interested in adding this feature once we understand it better. Could you please describe why you need this?

I'm using this code for truncate Instant (for period >= 1 second)

operator fun Instant.rem(period: Duration): Duration {
    val periodMillis = period.inWholeMilliseconds
    val thisMillis = toEpochMilliseconds()
    return (thisMillis % periodMillis).milliseconds
}

fun Instant.floorBy(period: Duration): Instant = this - this % period

I'm using it for make flow of current time for ui usage.

Would like to see at least Instant % Duration as standard feature

@dkhalanskyjb
Copy link
Collaborator

@demidenko, almost everything from #266 (comment) applies to your solution as well:

  • No time zone support, so for time zones with an offset like +03:30 calling rem with 1 hour will not work, but it seems like it does,
  • Calling rem with 24 hours looks like it will reset into midnight, but is in fact nonsense,
  • Almost all input values would lead to unpredictable results, like 7 seconds.

So, I'm not convinced it's worth introducing such a brittle operation just for truncation.

@dkhalanskyjb
Copy link
Collaborator

Closing in favor of #325

@dkhalanskyjb dkhalanskyjb closed this as not planned Won't fix, can't repro, duplicate, stale Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatters Related to parsing and formatting
Projects
None yet
Development

No branches or pull requests

4 participants
@demidenko @dkhalanskyjb @Stephen-Hamilton-C and others