Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v2]
On Wed, 24 Nov 2021 21:27:15 GMT, Joe Wang wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Refined wording > > src/java.base/share/classes/java/time/format/DateTimeFormatter.java line 509: > >> 507: * parsed from the zone name that does not imply daylight saving time, >> then >> 508: * {@link ChronoZonedDateTime#withLaterOffsetAtOverlap()} is issued >> 509: * to use the standard offset at the overlap, before forming the >> instant. > > Is the standard offset the subject instead of the withLaterOffsetAtOverlap > method? Calling the method seems to be an impl detail to me. We might > rephrase the sentence to sth. like: > If the {@code ZoneId} parsed does not indicate daylight saving time, the > standard offset will be used at the local time-line overlap as specified in > the {@link ChronoZonedDateTime#withLaterOffsetAtOverlap()} method to form the > instant. Removed the method, and made `standard offset` the subject of the sentence. > src/java.base/share/classes/java/time/format/Parsed.java line 139: > >> 137: * The parsed zone name type. >> 138: */ >> 139: int zoneNameType = -1; > > Could be an Enum if that helps with readability. Since the index needs some calculation, I left it as `int`. Changed to use the constant fields for better readability. - PR: https://git.openjdk.java.net/jdk/pull/6527
Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v2]
On Tue, 23 Nov 2021 23:50:36 GMT, Naoto Sato wrote: >> This fix intends to honor the type (std/dst/generic) of parsed zone name for >> selecting the offset at the overlap. Corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Refined wording Thanks, Joe. Refined the wording in the compatibility assessment in the CSR. - PR: https://git.openjdk.java.net/jdk/pull/6527
Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v2]
On Tue, 23 Nov 2021 23:50:36 GMT, Naoto Sato wrote: >> This fix intends to honor the type (std/dst/generic) of parsed zone name for >> selecting the offset at the overlap. Corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Refined wording For the compatibility assessment, it looks like an incompatible change since apps that expect DST will get a different offset at the overlap. The risk is low, perhaps because of the rare use case? That additional explanation might be helpful. src/java.base/share/classes/java/time/format/DateTimeFormatter.java line 509: > 507: * parsed from the zone name that does not imply daylight saving time, > then > 508: * {@link ChronoZonedDateTime#withLaterOffsetAtOverlap()} is issued > 509: * to use the standard offset at the overlap, before forming the instant. Is the standard offset the subject instead of the withLaterOffsetAtOverlap method? Calling the method seems to be an impl detail to me. We might rephrase the sentence to sth. like: If the {@code ZoneId} parsed does not indicate daylight saving time, the standard offset will be used at the local time-line overlap as specified in the {@link ChronoZonedDateTime#withLaterOffsetAtOverlap()} method to form the instant. src/java.base/share/classes/java/time/format/Parsed.java line 139: > 137: * The parsed zone name type. > 138: */ > 139: int zoneNameType = -1; Could be an Enum if that helps with readability. - PR: https://git.openjdk.java.net/jdk/pull/6527
Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v2]
> This fix intends to honor the type (std/dst/generic) of parsed zone name for > selecting the offset at the overlap. Corresponding CSR has also been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Refined wording - Changes: - all: https://git.openjdk.java.net/jdk/pull/6527/files - new: https://git.openjdk.java.net/jdk/pull/6527/files/40daaa29..99ad3cad Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6527=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6527=00-01 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6527.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6527/head:pull/6527 PR: https://git.openjdk.java.net/jdk/pull/6527