Improve the KDoc #367

dkhalanskyjb · 2024-03-12T14:24:04Z

Fixes #347

Not requesting the review initially, because I myself need to look at it with fresh eyes in a couple of days, but if someone's in the mood, they can point out the issues already. EDIT: after one more pass on my own, I've requested a review.

qwwdfsad

Tremendous and solid work

core/common/src/Clock.kt

core/common/src/DateTimePeriod.kt

core/common/src/DateTimeUnit.kt

core/common/src/Clock.kt

core/common/src/Instant.kt

qwwdfsad

A small batch from the stash

core/common/src/Clock.kt

core/common/src/Instant.kt

core/common/src/DateTimePeriod.kt

core/common/src/Clock.kt

core/common/src/DateTimePeriod.kt

kevincianfarini · 2024-04-06T00:45:40Z

core/common/src/DateTimePeriod.kt

+ * ```
+ *
+ * [parse] and [toString] methods can be used to obtain a [DateTimePeriod] from and convert it to a string in the
+ * ISO 8601 extended format.


Is there any way we can link to the ISO 8601 spec section which outlines time intervals?

Also, is it worth calling out what revision of ISO 8601 we're adhering to in every KDoc? For example, I previously misunderstood that LocalTime should support 24:00 because it was included in ISO 8601:2004(E) , but I found out it was removed in later revisions of the spec. Being specific about what revision we support could help remove ambiguity.

We can only mention the relevant ISO 8601 section, but not link to it, because the standard is not in public access.

core/common/src/TimeZone.kt

core/common/src/LocalDate.kt

core/common/test/samples/TimeZoneSamples.kt

core/common/src/format/DateTimeFormat.kt

ilya-g

The first part of comments

core/common/src/Clock.kt

ilya-g · 2024-04-15T10:54:38Z

core/common/src/Clock.kt

+ * **Pitfall**: using this function with [Clock.System] is error-prone,
+ * because [Clock.System] is not well suited for measuring time intervals.
+ * Please only use this conversion function on the [Clock] instances that are fully controlled programmatically.


It's good to describe specifics of a time source obtained from a clock, but I wouldn't call this error prone and would avoid giving this recommendation "Please only use this conversion function on the [Clock] instances that are fully controlled programmatically."

What other valid use cases do you see for this function? Related: #372

For example, tracking timeouts tied to the wall clock. TimeMarks obtained from Instants can potentially be persistable between program runs, however we may currently lack the API to achieve that.

core/common/src/DateTimePeriod.kt

ilya-g · 2024-04-17T13:46:52Z

core/common/src/DateTimeUnit.kt

+ * [DateTimePeriod] is a combination of all [DateTimeUnit] values, used to express things like
+ * "two days and three hours."


May be unclear what "all [DateTimeUnit] values" means given that we declare many date-time units and there can also be arbitrary user-defined ones.

core/common/src/DateTimeUnit.kt

ilya-g · 2024-04-17T14:05:41Z

core/common/src/Instant.kt

+ *
+ * For values very far in the past or the future, this conversion may fail.
+ * The specific range of values that can be converted to [LocalDateTime] is platform-specific, but at least
+ * [DISTANT_PAST], [DISTANT_FUTURE], and all values between them can be converted to [LocalDateTime].


another way to say it is "all values in the range [DISTANT_PAST]..[DISTANT_FUTURE]", up to your decision.

I'd like things like a..b to be between backticks so that .. is not treated as just some broken punctuation, but then, we can't have references to entities.
@qwwdfsad, IIRC, you're collecting feature requests for the KDoc syntax—navigable links in code blocks and inline code would be nice.

Kotlin/dokka#3571

Don't hesitate to file it in Dokka with kdoc-spec label

core/common/src/LocalDateTime.kt

core/common/src/DateTimePeriod.kt

qwwdfsad

🔥

Fixes #347

Co-authored-by: ilya-g <ilya.gorbunov@jetbrains.com>

See #386

ilya-g · 2024-05-01T14:14:30Z

core/common/src/LocalTime.kt

+ * ### Platform specifics
+ *
+ * On the JVM,
+ * there are `LocalTime.toJavaLocalTime()` and `java.time.LocalTime.toKotlinLocalTime()` extension functions.


Better to mention for what are these extensions. (same for other entities for which we provide conversions)

Tough for me to think of a good way to say this without being tautological, but I tried. Is this what you meant?

ilya-g · 2024-05-01T14:25:52Z

core/common/src/LocalTime.kt

@@ -49,9 +103,15 @@ public expect class LocalTime : Comparable<LocalTime> {
         * @throws IllegalArgumentException if [secondOfDay] is outside the `0 until 86400` range,
         * with 86400 being the number of seconds in a calendar day.
         *
+         * It is incorrect to pass the number of seconds since the start of the day to this function.


Together with the summary, it makes the impression that this function is inherently incorrect.

I believe that we should describe the model this entity represents and its discrepancies once in the class docs, and then refer to it instead of providing such note on each operation

True, the wording here is misleading. I tried correcting it.

core/common/src/UtcOffset.kt

core/common/src/format/LocalDateFormat.kt

core/common/test/samples/DateTimePeriodSamples.kt

core/common/src/Instant.kt

ilya-g · 2024-05-03T14:22:02Z

core/common/test/samples/InstantSamples.kt

+    @Test
+    fun plusDuration() {
+        // Finding a moment that's later than the starting point by the given amount of real time
+        val instant = Instant.fromEpochMilliseconds(epochMilliseconds = 7 * 60 * 60 * 1000)


It would be more understandable to use instants obtained from parse and show the result as toString for arithmetic operation samples.

Using fromEpochMilliseconds also serves to highlight the difference between Instant and LocalDateTime values. Newcomers are often confused by the distinction between them.

I fear that overreliance on from/toEpochMilliseconds in samples would convey the wrong idea that Instant is completely defined by that number of milliseconds.

Replaced with fromEpochSeconds/epochSeconds + nanosecondsOfSecond.

core/common/test/samples/InstantSamples.kt

ilya-g · 2024-05-03T14:28:05Z

core/common/test/samples/InstantSamples.kt

+        repeat(100) {
+            val instant1 = randomInstant()
+            val instant2 = randomInstant()
+            check((instant1 < instant2) == (instant1.toEpochMilliseconds() < instant2.toEpochMilliseconds()))


It looks like a definition of this operation, whereas it is more nuanced. Better to use constant instants obtained from parse

core/common/test/samples/InstantSamples.kt

core/common/test/samples/LocalDateSamples.kt

ilya-g · 2024-05-08T12:30:00Z

core/common/test/samples/InstantSamples.kt

+    @Test
+    fun plusDuration() {
+        // Finding a moment that's later than the starting point by the given amount of real time
+        val instant = Instant.fromEpochMilliseconds(epochMilliseconds = 7 * 60 * 60 * 1000)


I fear that overreliance on from/toEpochMilliseconds in samples would convey the wrong idea that Instant is completely defined by that number of milliseconds.

ilya-g

Great work!
Let's address what is clear and merge it, leaving debatable things for further improvement.

ilya-g · 2024-05-09T15:30:26Z

core/common/test/samples/format/DateTimeFormatSamples.kt

+        // Printing a given date-time format as a Kotlin code snippet that creates the same format
+        val customFormat = LocalDate.Format {
+            @OptIn(FormatStringsInDatetimeFormats::class)
+            byUnicodePattern("MM/dd uuuu")


Would be cool to show the substitution for yyyy which is more widespread instead.

dkhalanskyjb requested a review from qwwdfsad March 18, 2024 12:45

qwwdfsad reviewed Mar 20, 2024

View reviewed changes

dkhalanskyjb force-pushed the doc-improvements branch from 39d8627 to 2b88030 Compare March 21, 2024 14:24

dkhalanskyjb requested a review from qwwdfsad March 22, 2024 13:22

qwwdfsad reviewed Mar 26, 2024

View reviewed changes

core/common/src/Clock.kt Show resolved Hide resolved

core/common/src/Clock.kt Outdated Show resolved Hide resolved

core/common/src/Instant.kt Show resolved Hide resolved

qwwdfsad reviewed Mar 26, 2024

View reviewed changes

core/common/src/DateTimePeriod.kt Outdated Show resolved Hide resolved

core/common/src/DateTimePeriod.kt Show resolved Hide resolved

dkhalanskyjb mentioned this pull request Apr 3, 2024

Improved LocalDateTime documentation #248

Closed

lppedd reviewed Apr 3, 2024

View reviewed changes

core/common/src/Clock.kt Outdated Show resolved Hide resolved

lppedd reviewed Apr 3, 2024

View reviewed changes

core/common/src/Clock.kt Outdated Show resolved Hide resolved

kevincianfarini reviewed Apr 6, 2024

View reviewed changes

core/common/src/DateTimePeriod.kt Outdated Show resolved Hide resolved

kevincianfarini reviewed Apr 6, 2024

View reviewed changes

core/common/src/DateTimePeriod.kt Show resolved Hide resolved

kevincianfarini reviewed Apr 6, 2024

View reviewed changes

core/common/src/TimeZone.kt Outdated Show resolved Hide resolved

dkhalanskyjb force-pushed the doc-improvements branch from 7e8f55e to daec006 Compare April 16, 2024 12:22

qwwdfsad self-requested a review April 16, 2024 12:44

qwwdfsad reviewed Apr 16, 2024

View reviewed changes

core/common/src/LocalDate.kt Outdated Show resolved Hide resolved

core/common/src/LocalDate.kt Show resolved Hide resolved

core/common/test/samples/TimeZoneSamples.kt Show resolved Hide resolved

core/common/src/format/DateTimeFormat.kt Show resolved Hide resolved

ilya-g reviewed Apr 16, 2024

View reviewed changes

dkhalanskyjb force-pushed the doc-improvements branch from 9fd3885 to 8391eff Compare April 17, 2024 13:12

dkhalanskyjb requested review from qwwdfsad and ilya-g April 17, 2024 13:45

ilya-g reviewed Apr 17, 2024

View reviewed changes

dkhalanskyjb requested a review from ilya-g April 19, 2024 11:20

qwwdfsad approved these changes Apr 19, 2024

View reviewed changes

dkhalanskyjb added 7 commits April 29, 2024 13:08

Improve the KDoc

b6a6338

Fixes #347

Fix typos and small mistakes

0a7298e

Don't hyphenate ISO 8601 when used as a compound adjective

e076439

Implement the first pack of fixes after the review

ca9c739

The second pack of changes

c6dd47f

Add samples for TimeZone, UtcOffset, and Month

1a4b0f9

WIP: add samples for LocalTime

6883ffb

dkhalanskyjb and others added 6 commits April 29, 2024 13:10

Update core/common/src/DateTimeUnit.kt

fd8be7d

Co-authored-by: ilya-g <ilya.gorbunov@jetbrains.com>

Address the comments

1302b7e

Clarify setting DateTimeComponents.month

8f73a08

Provide a one-line description for every sample

33a3717

More additions according to the reviews

a2af2de

feat: new docs review

f7878af

dkhalanskyjb force-pushed the doc-improvements branch from 01650af to f7878af Compare April 29, 2024 11:48

dkhalanskyjb mentioned this pull request Apr 29, 2024

feat: new docs review #386

Closed

dkhalanskyjb added 2 commits April 30, 2024 10:36

Mention the relevant parts of ISO 8601 when describing formats

63644e6

Incorporate some more proofreading results

e6b758e

See #386

ilya-g reviewed May 1, 2024

View reviewed changes

Address the review

517ae7a

dkhalanskyjb requested a review from ilya-g May 3, 2024 09:31

ilya-g reviewed May 3, 2024

View reviewed changes

Address the review

f45e493

dkhalanskyjb requested a review from ilya-g May 3, 2024 15:15

~fixup

8a0e356

ilya-g reviewed May 8, 2024

View reviewed changes

ilya-g approved these changes May 9, 2024

View reviewed changes

dkhalanskyjb added 6 commits May 10, 2024 11:25

Remove separate samples for Long overloads (plus a small fixup)

f70e422

Use yyyy instead of uuuu in the Unicode pattern sample

6c1423a

Use Instant.fromEpochSeconds in samples more

fa59e05

Fix a broken sample reference

a529d44

Fix broken table rendering

a0a1335

Properly display samples in Dokka

8851274

ilya-g approved these changes May 13, 2024

View reviewed changes

dkhalanskyjb merged commit cff3fdd into master May 13, 2024
1 check passed

dkhalanskyjb deleted the doc-improvements branch May 13, 2024 11:12

qwwdfsad mentioned this pull request May 27, 2024

Improve the API reference Kotlin/kotlinx.coroutines#4133

Open

danil-pavlov mentioned this pull request Jul 11, 2024

update: second text review #400

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the KDoc #367

Improve the KDoc #367

dkhalanskyjb commented Mar 12, 2024 •

edited

Loading

qwwdfsad left a comment

qwwdfsad left a comment

kevincianfarini Apr 6, 2024 •

edited

Loading

kevincianfarini Apr 6, 2024

dkhalanskyjb Apr 30, 2024

ilya-g left a comment

ilya-g Apr 15, 2024

dkhalanskyjb Apr 17, 2024

ilya-g Apr 17, 2024

ilya-g Apr 17, 2024

ilya-g Apr 17, 2024

dkhalanskyjb Apr 19, 2024

qwwdfsad Apr 19, 2024

qwwdfsad left a comment

ilya-g May 1, 2024

dkhalanskyjb May 1, 2024

ilya-g May 1, 2024

dkhalanskyjb May 1, 2024

ilya-g May 3, 2024

dkhalanskyjb May 3, 2024

ilya-g May 8, 2024

dkhalanskyjb May 10, 2024

ilya-g May 3, 2024

ilya-g May 8, 2024

ilya-g left a comment

ilya-g May 9, 2024

dkhalanskyjb May 10, 2024

		* [DateTimePeriod] is a combination of all [DateTimeUnit] values, used to express things like
		* "two days and three hours."

Improve the KDoc #367

Improve the KDoc #367

Conversation

dkhalanskyjb commented Mar 12, 2024 • edited Loading

qwwdfsad left a comment

Choose a reason for hiding this comment

qwwdfsad left a comment

Choose a reason for hiding this comment

kevincianfarini Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qwwdfsad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb commented Mar 12, 2024 •

edited

Loading

kevincianfarini Apr 6, 2024 •

edited

Loading