feat: new docs review #386
Conversation
Fixes #347
dkhalanskyjb
force-pushed
the
doc-improvements
branch
from
01650af
to
f7878af
Compare
There was a problem hiding this comment.
Thank you! I've merged most of the changes proposed here directly to #367.
| @@ -77,7 +77,7 @@ import kotlinx.serialization.Serializable | |||
| * LocalDateTime(date, time) | |||
| * ``` | |||
| * | |||
| * Some additional constructors that accept the date's and time's fields directly are provided for convenience. | |||
| * Some additional constructors that accept the date and time fields directly are provided for convenience. | |||
There was a problem hiding this comment.
The idea here is that you can either pass the contents of the date and time fields (like above) or pass the fields of the date and the fields of the time. Hence the possessive form. Without it, I don't think the difference between the two approaches is highlighted. Maybe there's a better way to word this.
There was a problem hiding this comment.
To avoid confusion, I think it's better to write it explicitly:
"Some additional constructors that directly accept the values (or content) from date and time fields are provided for convenience."
| * This class does not describe specific *moments in time*, which are represented as [Instant] values. | ||
| * Instead, its instances can be thought of as clock readings, something that someone could observe in their time zone. | ||
| * For example, `2020-08-30T18:43` is not a *moment in time*, since someone in Berlin and someone in Tokyo would witness | ||
| * This class does not describe specific *moments in time* represented as [Instant] values. |
There was a problem hiding this comment.
The idea: "This class does not describe specific moments in time. Instant is the thing that represents moments in time; if you reach for LocalDateTime for this use case, don't; use Instant instead". I think the proposed version doesn't convey that.
There was a problem hiding this comment.
Thank you, what about "This class does not describe specific moments in time. For that, use [Instant] values instead."
| @@ -13,7 +13,7 @@ import kotlinx.serialization.Serializable | |||
| * The date part of [LocalDateTime]. | |||
| * | |||
| * This class represents dates without a reference to a particular time zone. | |||
| * As such, these objects may denote different spans of time in different time zones: for someone in Berlin, | |||
| * As such, these objects may denote different periods in different time zones: for someone in Berlin, | |||
There was a problem hiding this comment.
This is probably a correct change, but this could interfere with us already having the concept of a "period" in the library: our "periods" are things like "1 day, 2 hours", without an established starting point, but here, the word "period" is used in the sense of "the period between 2020 and 2021", not "the period of 4 years". Is there some other phrasing that's natural but doesn't interfere with the terminology we establish?
There was a problem hiding this comment.
I see, maybe 'time intervals' or 'time frames' would fit better here
| @@ -218,7 +218,7 @@ public expect class LocalDate : Comparable<LocalDate> { | |||
|
|
|||
| /** | |||
| * Compares `this` date with the [other] date. | |||
| * Returns zero if this date represents the same day as the other (i.e., equal to other), | |||
| * Returns zero if this date represents the same day as the other (for example, equal to other), | |||
There was a problem hiding this comment.
That's not "e.g.", that's "i.e."
We could replace this with something like "in other words" if "i.e." is undesirable.
There was a problem hiding this comment.
Thank you for the clarification, here are several options:
(meaning they're equal to each other)
(with equal values)
(equal to one another)
|
Added the new suggestions in e6b758e |
|
Most of the proposed changes were already merged, and rebasing this PR would be difficult, so there seems to be no reason to keep it. |
No description provided.