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