On Fri, 15 Apr 2022 18:47:53 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Supporting `IsoFields` temporal fields in chronologies that are similar to >> ISO chronology. Corresponding CSR has also been drafted. > > Naoto Sato 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 15 additional commits since > the last revision: > > - Revert to use the default method > - Merge branch 'master' into JDK-8279185 > - Removed unnecessary package names > - Modified class desc of `IsoFields` > - abstract class -> top level interface > - interface -> abstract class > - Merge branch 'master' into JDK-8279185 > - Removed the method > - Merge branch 'master' into JDK-8279185 > - Changed to use a type to determine ISO based or not > - ... and 5 more: > https://git.openjdk.java.net/jdk/compare/5ee1a816...82339ec6 src/java.base/share/classes/java/time/chrono/Chronology.java line 794: > 792: * @since 19 > 793: */ > 794: default boolean isIsoBased() { Can the description be more specific. Each of the chronologies mentioned say they have the same number of months, the number of days in each month, and day-of-year and leap years are the same as ISO. The week-based fields in IsoFields also depend ISO defined concepts. Perhaps it should say that this method should return true only if all of the fields of IsoFields are supported for the chronology. The chronology names could be links and omit the @see tags, including ISOChronology. In the @return add ", otherwise return {@code false}." src/java.base/share/classes/java/time/temporal/IsoFields.java line 600: > 598: if (!isIsoBased(temporal)) { > 599: throw new DateTimeException("Resolve requires ISO based > chronology: " + > 600: Chronology.from(temporal)); The name change doesn't add much, I'd leave both `ensureIso` and `isIso` method names unchanged. src/java.base/share/classes/java/time/temporal/IsoFields.java line 739: > 737: > 738: static boolean isIsoBased(TemporalAccessor temporal) { > 739: return Chronology.from(temporal).isIsoBased(); Can this method be private static? Also, move the `ensureIso` method to be next to `isIso`. ------------- PR: https://git.openjdk.java.net/jdk/pull/7683
