LocalTime#toSecondOfDay & LocalTime#ofSecondOfDay. #204
Conversation
There was a problem hiding this comment.
Indeed, it seems convenient to have such conversion functions.
Two points though:
- the underlying value is a number of nanoseconds since the beginning of day, so it would be consistent to be able to get both nanoseconds of day and seconds of day;
- naming should be discussed further, so it is consistent with other functions/properties.
- for example, additional factory methods in
Instantare calledfromSomething(...). - the opposite conversion methods can be either functions, like
toNanosecondsOfDay(), or properties, likenanosecondsOfDay. - singular or plural form of seconds/nanoseconds
- for example, additional factory methods in
core/common/src/LocalTime.kt
Outdated
| /** | ||
| * Returns a LocalTime with the specified [secondOfDay]. The nanosecond field will be set to zero. | ||
| */ | ||
| public fun ofSecondOfDay(secondOfDay: Int): LocalTime |
There was a problem hiding this comment.
It should be documented when and what this function throws with the @throws documentation block.
I presume it should be an IllegalArgumentException.
There should be a test checking that such exception or its inheritor is indeed thrown in case of invalid input.
vanniktech
force-pushed
the
local-time-to-second-of-day
branch
from
7da608e to
5163e27
Compare
There was a problem hiding this comment.
After an internal discussion, we decided to have fromSecondOfDay/toSecondOfDay and fromNanosecondOfDay/toNanosecondOfDay pairs (that is, the accessors shouldn't be properties).
We also decided that we want [to/from]MillisecondOfDay as well, but if you don't feel like adding it, we'll do it on our own. Also, there's native-only ThreeTenBpLocalTimeTest that has tests for the new functions, and some of them could be ported to common code, but this is also something that could be easier for us to do.
core/common/src/LocalTime.kt
Outdated
| /** Returns the time as a second of a day, from 0 to 24 * 60 * 60 - 1. */ | ||
| public val secondOfDay: Int | ||
|
|
||
| /** Returns the time as a nanosecond of a day, from 0 to 24 * 60 * 60 * 1000 - 1. */ |
There was a problem hiding this comment.
| /** Returns the time as a nanosecond of a day, from 0 to 24 * 60 * 60 * 1000 - 1. */ | |
| /** Returns the time as a nanosecond of a day, from 0 to 24 * 60 * 60 * 1_000_000_000 - 1. */ |
core/common/test/LocalTimeTest.kt
Outdated
| 0L to LocalTime(0, 0), | ||
| 5000000001L to LocalTime(0, 0, 5, 1), | ||
| 44105000000100L to LocalTime(12, 15, 5, 100), | ||
| 86399000000999L to LocalTime(23, 59, 59, 999), |
There was a problem hiding this comment.
All of these have the millisecond and microsecond triads zeroed. I'd add a test with 123456789 nanoseconds or something like that.
| fun fromNanosecondOfDayInvalid() { | ||
| LocalTime.fromNanosecondOfDay(0) | ||
| assertFailsWith<IllegalArgumentException> { LocalTime.fromNanosecondOfDay(-1) } | ||
| assertFailsWith<IllegalArgumentException> { LocalTime.fromNanosecondOfDay(Long.MAX_VALUE) } |
There was a problem hiding this comment.
A good idea is to check the boundaries. For example, in the test above, in addition to (or instead of) testing 86399000000999L, 86399999999999L, the maximum admissible value, could be tested, and here we could test 86400000000000L, the first non-admissible value.
|
Regarding |
| /** | ||
| * Returns a LocalTime with the specified [secondOfDay]. The nanosecond field will be set to zero. | ||
| * | ||
| * @throws IllegalArgumentException if the boundaries of [secondOfDay] are exceeded. |
There was a problem hiding this comment.
The reference is broken now.
There was a problem hiding this comment.
It's referencing the parameter. Also works for me.
Motivation: I'm storing my LocalTime using second of day to leverage sql sorting