On Fri, 16 Jun 2023 22:12:43 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> In java.time packages, clarify timeline order javadoc to mention "before"
>> and "after" in the value of the `compareTo` method return values.
>> Add javadoc @see tags to isBefore and isAfter methods
>>
>> Replace use of "negative" and positive with "less than zero" and "greater
>> than zero" in javadoc @return
>> The term "positive" is ambiguous, zero is considered positive and indicates
>> equality.
>
> Roger Riggs has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Use {@code xxx} to highlight the comparison against the arg.
> Update copyrights.
> - Merge branch 'master' into 8310033-time-compareto
> - Clarify for Duration, AbstractChronology, and Chronology
> - Correct javadoc of compareInstant
> - 8310033: Improve Instant.compareTo javadoc to mention before and after
> Refine timeline order to mention before and after
> Add javadoc @see tags to isBefore and isAfter methods
As things stand, this PR makes things worse not better I'm afraid.
src/java.base/share/classes/java/time/Duration.java line 1422:
> 1420: *
> 1421: * @param otherDuration the other duration to compare to, not null
> 1422: * @return the comparator value is less than zero if the {@code
> otherDuration} is before,
There is no concept of before and after when talking about a `Duration`
src/java.base/share/classes/java/time/Instant.java line 1276:
> 1274: * @param otherInstant the other instant to compare to, not null
> 1275: * @return the comparator value is less than zero if the {@code
> otherInstant} is before,
> 1276: * zero if they are equal, greater than zero if the {@code
> otherInstant} is after
Suggestion:
* @return the comparator value - less than zero if {@code otherInstant} is
before this instant,
* zero if they are equal, greater than zero if {@code
otherInstant} is after this instant
Saying `the {@code otherInstant}` doesn't read correctly in text.
`the comparator value` needs punctuation after it in order to make sense
src/java.base/share/classes/java/time/MonthDay.java line 678:
> 676: *
> 677: * @param other the other month-day to compare to, not null
> 678: * @return the comparator value is less than zero if the {@code
> other} is before,
Using before/after here could be confusing, as January could be considered to
be before or after July (since the year is not defined).
src/java.base/share/classes/java/time/OffsetDateTime.java line 1805:
> 1803: *
> 1804: * @param other the other date-time to compare to, not null
> 1805: * @return the comparator value is less than zero if the {@code
> other} is before,
As per `OffsetTime` this definition is wrong. The comparison order is not
before/after.
src/java.base/share/classes/java/time/OffsetTime.java line 1284:
> 1282: *
> 1283: * @param other the other time to compare to, not null
> 1284: * @return the comparator value is less than zero if the {@code
> other} is before,
Using before/after here is very wrong. As described in the text above, two
instances can be at the same point in time, yet still have a defined sort order.
src/java.base/share/classes/java/time/ZoneOffset.java line 717:
> 715: *
> 716: * @param other the other date to compare to, not null
> 717: * @return the comparator value is less than zero if the {@code
> other} is before,
It is tricky to describe an offset as before or after. If you are going to
change this you will need a much better description
src/java.base/share/classes/java/time/chrono/AbstractChronology.java line 659:
> 657: *
> 658: * @param other the other chronology to compare to, not null
> 659: * @return the comparator value is less than zero if the {@code
> other} ID string is before,
A `Chronology` is compared by ID, not before/after
src/java.base/share/classes/java/time/chrono/ChronoZonedDateTime.java line 572:
> 570: *
> 571: * @param other the other date-time to compare to, not null
> 572: * @return the comparator value is less than zero if the {@code
> other} is before,
The comparison is not before/after, as the ID is taken into account
src/java.base/share/classes/java/time/chrono/Chronology.java line 810:
> 808: *
> 809: * @param other the other chronology to compare to, not null
> 810: * @return the comparator value is less than zero if the {@code
> other} ID string is before,
Not before/after
src/java.base/share/classes/java/time/zone/ZoneOffsetTransition.java line 403:
> 401: *
> 402: * @param transition the transition to compare to, not null
> 403: * @return the comparator value is less than zero if the {@code
> transition} is before,
Not before/after
-------------
Changes requested by scolebourne (Author).
PR Review: https://git.openjdk.org/jdk/pull/14479#pullrequestreview-1485184785
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233371176
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233371665
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372314
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372199
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372018
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233371857
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233371782
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372453
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372462
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1233372480
