Implement the first pack of fixes after the review

Author: dkhalanskyjb

core/common/src/Clock.kt

@@ -11,6 +11,11 @@ import kotlin.time.*
  * A source of [Instant] values.
  *
  * See [Clock.System][Clock.System] for the clock instance that queries the operating system.
+ *
+ * It is recommended not to use [Clock.System] directly in the implementation; instead, one could pass a
+ * [Clock] explicitly to the functions or classes that need it.
+ * This way, tests can be written deterministically by providing custom [Clock] implementations
+ * to the system under test.
  */
 public interface Clock {
     /**
@@ -23,7 +28,7 @@ public interface Clock {
     public fun now(): Instant
 
     /**
-     * The [Clock] instance that queries the operating system as its source of time knowledge.
+     * The [Clock] instance that queries the platform-specific system clock as its source of time knowledge.
      *
      * Successive calls to [now] will not necessarily return increasing [Instant] values, and when they do,
      * these increases will not necessarily correspond to the elapsed time.
@@ -35,6 +40,9 @@ public interface Clock {
      *
      * When predictable intervals between successive measurements are needed, consider using
      * [TimeSource.Monotonic].
+     *
+     * For improved testability, one could avoid using [Clock.System] directly in the implementation,
+     * instead passing a [Clock] explicitly.
      */
     public object System : Clock {
         override fun now(): Instant = @Suppress("DEPRECATION_ERROR") Instant.now()
@@ -47,6 +55,15 @@ public interface Clock {
 
 /**
  * Returns the current date at the given [time zone][timeZone], according to [this Clock][this].
+ *
+ * The time zone is important because the current date is not the same in all time zones at the same time.
+ * ```
+ * val clock = object : Clock {
+ *    override fun now(): Instant = Instant.parse("2020-01-01T12:00:00Z")
+ * }
+ * val dateInUTC = clock.todayIn(TimeZone.UTC) // 2020-01-01
+ * val dateInNewYork = clock.todayIn(TimeZone.of("America/New_York")) // 2019-12-31
+ * ```
  */
 public fun Clock.todayIn(timeZone: TimeZone): LocalDate =
     now().toLocalDateTime(timeZone).date

core/common/src/DateTimePeriod.kt

@@ -17,9 +17,22 @@ import kotlinx.serialization.Serializable
 /**
  * A difference between two [instants][Instant], decomposed into date and time components.
  *
- * The date components are: [years], [months], [days].
+ * The date components are: [years] ([DateTimeUnit.YEAR]), [months] ([DateTimeUnit.MONTH]), [days] ([DateTimeUnit.DAY]).
  *
- * The time components are: [hours], [minutes], [seconds], [nanoseconds].
+ * The time components are: [hours] ([DateTimeUnit.HOUR]), [minutes] ([DateTimeUnit.MINUTE]),
+ * [seconds] ([DateTimeUnit.SECOND]), [nanoseconds] ([DateTimeUnit.NANOSECOND]).
+ *
+ * The time components are not independent and always overflow into one another.
+ * Likewise, months overflow into years.
+ * For example, there is no difference between `DateTimePeriod(months = 24, hours = 2, minutes = 63)` and
+ * `DateTimePeriod(years = 2, hours = 3, minutes = 3)`.
+ *
+ * All components can also be negative: for example, `DateTimePeriod(months = -5, days = 6, hours = -3)`.
+ * Whereas `months = 5` means "5 months after," `months = -5` means "5 months earlier."
+ *
+ * Since, semantically, a [DateTimePeriod] is a combination of [DateTimeUnit] values, in cases when the period is a
+ * fixed time interval (like "yearly" or "quarterly"), please consider using [DateTimeUnit] directly instead:
+ * for example, instead of `DateTimePeriod(months = 6)`, one could use `DateTimeUnit.MONTH * 6`.
  *
  * ### Interaction with other entities
  *
@@ -29,6 +42,10 @@ import kotlinx.serialization.Serializable
  * [DatePeriod] is a subtype of [DateTimePeriod] that only stores the date components and has all time components equal
  * to zero.
  *
+ * [DateTimePeriod] can be thought of as a combination of a [Duration] and a [DatePeriod], as it contains both the
+ * time components of [Duration] and the date components of [DatePeriod].
+ * [Duration.toDateTimePeriod] can be used to convert a [Duration] to the corresponding [DateTimePeriod].
+ *
  * ### Construction, serialization, and deserialization
  *
  * When a [DateTimePeriod] is constructed in any way, a [DatePeriod] value, which is a subtype of [DateTimePeriod],
@@ -98,7 +115,17 @@ public sealed class DateTimePeriod {
     /**
      * Converts this period to the ISO 8601 string representation for durations, for example, `P2M1DT3H`.
      *
-     * @see DateTimePeriod.parse
+     * Note that the ISO 8601 duration is not the same as [Duration],
+     * but instead includes the date components, like [DateTimePeriod] does.
+     *
+     * Examples of the output:
+     * - `P2Y4M-1D`: two years, four months, minus one day;
+     * - `-P2Y4M1D`: minus two years, minus four months, minus one day;
+     * - `P1DT3H2M4.123456789S`: one day, three hours, two minutes, four seconds, 123456789 nanoseconds;
+     * - `P1DT-3H-2M-4.123456789S`: one day, minus three hours, minus two minutes,
+     *   minus four seconds, minus 123456789 nanoseconds;
+     *
+     * @see DateTimePeriod.parse for the detailed description of the format.
      */
     override fun toString(): String = buildString {
         val sign = if (allNonpositive()) { append('-'); -1 } else 1
@@ -144,16 +171,38 @@ public sealed class DateTimePeriod {
     public companion object {
         /**
          * Parses a ISO 8601 duration string as a [DateTimePeriod].
+         *
          * If the time components are absent or equal to zero, returns a [DatePeriod].
          *
-         * Additionally, we support the `W` signifier to represent weeks.
+         * Note that the ISO 8601 duration is not the same as [Duration],
+         * but instead includes the date components, like [DateTimePeriod] does.
          *
          * Examples of durations in the ISO 8601 format:
          * - `P1Y40D` is one year and 40 days
          * - `-P1DT1H` is minus (one day and one hour)
          * - `P1DT-1H` is one day minus one hour
          * - `-PT0.000000001S` is minus one nanosecond
          *
+         * The format is defined as follows:
+         * - First, optionally, a `-` or `+`.
+         *   If `-` is present, the whole period after the `-` is negated: `-P-2M1D` is the same as `P2M-1D`.
+         * - Then, the letter `P`.
+         * - Optionally, the number of years, followed by `Y`.
+         * - Optionally, the number of months, followed by `M`.
+         * - Optionally, the number of weeks, followed by `W`.
+         *   This is not a part of the ISO 8601 format but an extension.
+         * - Optionally, the number of days, followed by `D`.
+         * - The string can end here if there are no more time components.
+         *   If there are time components, the letter `T` is required.
+         * - Optionally, the number of hours, followed by `H`.
+         * - Optionally, the number of minutes, followed by `M`.
+         * - Optionally, the number of seconds, followed by `S`.
+         *   Seconds can optionally have a fractional part with up to nine digits.
+         *   The fractional part is separated with a `.`.
+         *
+         * All numbers can be negative, in which case, `-` is prepended to them.
+         * Otherwise, a number can have `+` prepended to it, which does not have an effect.
+         *
          * @throws IllegalArgumentException if the text cannot be parsed or the boundaries of [DateTimePeriod] are
          * exceeded.
          */
@@ -348,10 +397,14 @@ public class DatePeriod internal constructor(
      * Constructs a new [DatePeriod].
      *
      * It is recommended to always explicitly name the arguments when constructing this manually,
-     * like `DatePeriod(years = 1, months = 12)`.
+     * like `DatePeriod(years = 1, months = 12, days = 16)`.
      *
      * The passed numbers are not stored as is but are normalized instead for human readability, so, for example,
-     * `DateTimePeriod(months = 24)` becomes `DateTimePeriod(years = 2)`.
+     * `DateTimePeriod(months = 24, days = 41)` becomes `DateTimePeriod(years = 2, days = 41)`.
+     *
+     * If only a single component is set and is always non-zero and is semantically a fixed time interval
+     * (like "yearly" or "quarterly"), please consider using a multiple of [DateTimeUnit.DateBased] instead.
+     * For example, instead of `DatePeriod(months = 6)`, one can use `DateTimeUnit.MONTH * 6`.
      *
      * @throws IllegalArgumentException if the total number of months in [years] and [months] overflows an [Int].
      */
@@ -435,10 +488,14 @@ internal fun buildDateTimePeriod(totalMonths: Int = 0, days: Int = 0, totalNanos
  * Constructs a new [DateTimePeriod]. If all the time components are zero, returns a [DatePeriod].
  *
  * It is recommended to always explicitly name the arguments when constructing this manually,
- * like `DateTimePeriod(years = 1, months = 12)`.
+ * like `DateTimePeriod(years = 1, months = 12, days = 16)`.
  *
  * The passed numbers are not stored as is but are normalized instead for human readability, so, for example,
- * `DateTimePeriod(months = 24)` becomes `DateTimePeriod(years = 2)`.
+ * `DateTimePeriod(months = 24, days = 41)` becomes `DateTimePeriod(years = 2, days = 41)`.
+ *
+ * If only a single component is set and is always non-zero and is semantically a fixed time interval
+ * (like "yearly" or "quarterly"), please consider using a multiple of [DateTimeUnit] instead.
+ * For example, instead of `DateTimePeriod(months = 6)`, one can use `DateTimeUnit.MONTH * 6`.
  *
  * @throws IllegalArgumentException if the total number of months in [years] and [months] overflows an [Int].
  * @throws IllegalArgumentException if the total number of months in [hours], [minutes], [seconds] and [nanoseconds]
@@ -465,6 +522,10 @@ public fun DateTimePeriod(
  * The reason is that even a [Duration] obtained via [Duration.Companion.days] just means a multiple of 24 hours,
  * whereas in `kotlinx-datetime`, a day is a calendar day, which can be different from 24 hours.
  * See [DateTimeUnit.DayBased] for details.
+ *
+ * ```
+ * 2.days.toDateTimePeriod() // 0 days, 48 hours
+ * ```
  */
 // TODO: maybe it's more consistent to throw here on overflow?
 public fun Duration.toDateTimePeriod(): DateTimePeriod = buildDateTimePeriod(totalNanoseconds = inWholeNanoseconds)

core/common/src/Instant.kt

@@ -38,7 +38,7 @@ import kotlin.time.*
  * The [Clock.System] implementation uses the platform-specific system clock to obtain the current moment.
  * Note that this clock is not guaranteed to be monotonic, and it may be adjusted by the user or the system at any time,
  * so it should not be used for measuring time intervals.
- * For that, consider [TimeSource.Monotonic].
+ * For that, consider using [TimeSource.Monotonic] and [TimeMark] instead of [Clock.System] and [Instant].
  *
  * ### Obtaining human-readable representations
  *
@@ -104,7 +104,11 @@ import kotlin.time.*
  * requiring a [TimeZone]:
  *
  * ```
- * Clock.System.now().plus(1, DateTimeUnit.DAY, TimeZone.of("Europe/Berlin")) // one day from now in Berlin
+ * // one day from now in Berlin
+ * Clock.System.now().plus(1, DateTimeUnit.DAY, TimeZone.of("Europe/Berlin"))
+ *
+ * // a day and two hours short from two months later in Berlin
+ * Clock.System.now().plus(DateTimePeriod(months = 2, days = -1, hours = -2), TimeZone.of("Europe/Berlin"))
  * ```
  *
  * The difference between [Instant] values in terms of calendar-based units can be obtained using the [periodUntil]
@@ -366,6 +370,13 @@ public fun String.toInstant(): Instant = Instant.parse(this)
  * Returns an instant that is the result of adding components of [DateTimePeriod] to this instant. The components are
  * added in the order from the largest units to the smallest, i.e. from years to nanoseconds.
  *
+ * - If the [DateTimePeriod] only contains time-based components, please consider adding a [Duration] instead,
+ *   as in `Clock.System.now() + 5.hours`.
+ *   Then, it will not be necessary to pass the [timeZone].
+ * - If the [DateTimePeriod] only has a single non-zero component (only the months or only the days),
+ *   please consider using a multiple of [DateTimeUnit.DAY] or [DateTimeUnit.MONTH], like in
+ *   `Clock.System.now().plus(5, DateTimeUnit.DAY, TimeZone.currentSystemDefault())`.
+ *
  * @throws DateTimeArithmeticException if this value or the results of intermediate computations are too large to fit in
  * [LocalDateTime].
  */
@@ -375,6 +386,13 @@ public expect fun Instant.plus(period: DateTimePeriod, timeZone: TimeZone): Inst
  * Returns an instant that is the result of subtracting components of [DateTimePeriod] from this instant. The components
  * are subtracted in the order from the largest units to the smallest, i.e. from years to nanoseconds.
  *
+ * - If the [DateTimePeriod] only contains time-based components, please consider subtracting a [Duration] instead,
+ *   as in `Clock.System.now() - 5.hours`.
+ *   Then, it is not necessary to pass the [timeZone].
+ * - If the [DateTimePeriod] only has a single non-zero component (only the months or only the days),
+ *   please consider using a multiple of [DateTimeUnit.DAY] or [DateTimeUnit.MONTH], as in
+ *   `Clock.System.now().minus(5, DateTimeUnit.DAY, TimeZone.currentSystemDefault())`.
+ *
  * @throws DateTimeArithmeticException if this value or the results of intermediate computations are too large to fit in
  * [LocalDateTime].
  */

core/common/test/ClockTimeSourceTest.kt

@@ -83,4 +83,4 @@ class ClockTimeSourceTest {
         assertFailsWith<IllegalArgumentException> { markFuture - Duration.INFINITE }
         assertFailsWith<IllegalArgumentException> { markPast + Duration.INFINITE }
     }
-}
+}