RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT should handle offsets
Naoto Sato
naoto.sato at oracle.com
Mon Oct 1 17:22:40 UTC 2018
+1. Added myself as a reviewer of the CSR. Thanks for working on this.
Naoto
On 10/1/18 9:38 AM, Pallavi Sonal wrote:
> Thanks Roger, made the suggested changes.
>
> Thanks,
>
> Pallavi Sonal
>
> *From:*Roger Riggs
> *Sent:* Monday, October 01, 2018 9:29 PM
> *To:* Pallavi Sonal <pallavi.sonal at oracle.com>; Naoto Sato
> <naoto.sato at oracle.com>; core-libs-dev <core-libs-dev at openjdk.java.net>
> *Subject:* Re: RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT should
> handle offsets
>
> Hi,
>
> The checkbox for compatibility Kind Behavioral should be checked. (You
> may need to hit Edit to see it).
> A simple description of the compatibility risk may help clarify minimal.
> Something to the effect, that there is no change in formatting behavior
> and that parsing will now accept a numeric offset.
>
> Otherwise, looks fine.
>
> Thanks, Roger
>
> On 10/01/2018 09:24 AM, Pallavi Sonal wrote:
>
> Hi Naoto,
>
> Here is the CSR for review :
>
> https://bugs.openjdk.java.net/browse/JDK-8211316
>
> Thanks,
>
> Pallavi Sonal
>
> -----Original Message-----
>
> From: Naoto Sato
>
> Sent: Friday, September 28, 2018 9:38 PM
>
> To: Pallavi Sonal<pallavi.sonal at oracle.com> <mailto:pallavi.sonal at oracle.com>; Roger Riggs<roger.riggs at oracle.com> <mailto:roger.riggs at oracle.com>; core-libs-dev<core-libs-dev at openjdk.java.net> <mailto:core-libs-dev at openjdk.java.net>
>
> Subject: Re: RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT should handle offsets
>
> Hi Pallavi,
>
> Please file a CSR for this, as this will change the behavior.
>
> Naoto
>
> On 9/28/18 8:53 AM, Pallavi Sonal wrote:
>
> Thanks Roger. I will update it before the commit.
>
> -----Original Message-----
>
> From: Roger Riggs
>
> Sent: Friday, September 28, 2018 6:15 PM
>
> To: Pallavi Sonal<pallavi.sonal at oracle.com> <mailto:pallavi.sonal at oracle.com>; core-libs-dev
>
> <core-libs-dev at openjdk.java.net>
> <mailto:core-libs-dev at openjdk.java.net>
>
> Subject: Re: RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT should
>
> handle offsets
>
> Hi Pallavi,
>
> Looks fine,
>
> But can you re-wrap the lines with the new links, they got longer than
>
> 100 chars with the link.
>
> No new webrev is needed.
>
> Thanks, Roger
>
> On 9/28/18 8:35 AM, Pallavi Sonal wrote:
>
> Thanks Roger and Stephen. Here is the updated webrev with the suggested changes:
>
> http://cr.openjdk.java.net/~rpatil/8166138/webrev.04/
> <http://cr.openjdk.java.net/%7Erpatil/8166138/webrev.04/>
>
> Thanks,
>
> Pallavi Sonal
>
> -----Original Message-----
>
> From: Roger Riggs
>
> Sent: Friday, September 28, 2018 12:51 AM
>
> To: core-libs-dev<core-libs-dev at openjdk.java.net>
> <mailto:core-libs-dev at openjdk.java.net>
>
> Subject: Re: RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT should
>
> handle offsets
>
> Hi Pallavi,
>
> I'd suggest using @link for the reference (in both files).
>
> It will be easier for the reader to traverse and understand the pattern.
>
> DateTimeFormatterBuilder.java: line 836.
>
> The trailing "</p>" should be omitted so the readability of the source is maintained.
>
> Otherwise, looks good,
>
> Thanks, Roger
>
> On 09/27/2018 03:39 AM, Stephen Colebourne wrote:
>
> In DateTimeFormatter you need to qualify the @code part to refer to
>
> DateTimeFormatterBuilder.
>
> Otherwise good.
>
> thanks
>
> Stephen
>
> On Thu, 27 Sep 2018 at 05:35, Pallavi Sonal<pallavi.sonal at oracle.com> <mailto:pallavi.sonal at oracle.com> wrote:
>
> Thanks for the clarification. Here is the updated webrev for review :
>
> http://cr.openjdk.java.net/~rpatil/8166138/webrev.03/ <http://cr.openjdk.java.net/%7Erpatil/8166138/webrev.03/>
>
> Thanks,
>
> Pallavi Sonal
>
> -----Original Message-----
>
> From: Stephen Colebourne<scolebourne at joda.org> <mailto:scolebourne at joda.org>
>
> Sent: Tuesday, September 25, 2018 4:39 PM
>
> To: core-libs-dev<core-libs-dev at openjdk.java.net>
> <mailto:core-libs-dev at openjdk.java.net>
>
> Subject: Re: RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT
>
> should handle offsets
>
> I think it makes sense for both, although I was only considering
>
> appendInstant() when I wrote it.
>
> Stephen
>
> On Tue, 25 Sep 2018 at 09:27, Pallavi Sonal<pallavi.sonal at oracle.com>
> <mailto:pallavi.sonal at oracle.com> wrote:
>
> Hi Stephen,
>
> Is the addition to the documentation in your mail below meant for only appendInstant() method or for DateTimeFormatter.ISO_INSTANT as well ?
>
> -----Original Message-----
>
> From: Stephen Colebourne<scolebourne at joda.org> <mailto:scolebourne at joda.org>
>
> Sent: Sunday, September 23, 2018 12:36 PM
>
> To: core-libs-dev<core-libs-dev at openjdk.java.net>
> <mailto:core-libs-dev at openjdk.java.net>
>
> Subject: Re: RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT
>
> should handle offsets
>
> Thanks
>
> Can we change the docs to:
>
> <p>
>
> When formatting, the instant will always be suffixed by 'Z' to indicate UTC.
>
> When parsing, the behaviour of {@code appendOffsetId()} will be used to parse the offset, converting the instant to UTC as necessary.
>
> thanks
>
> Stephen
>
> On Fri, 21 Sep 2018 at 13:26, Pallavi Sonal<pallavi.sonal at oracle.com>
> <mailto:pallavi.sonal at oracle.com> wrote:
>
> Thank you Stephen for your inputs. Based on that, here is the updated webrev for review :
>
> http://cr.openjdk.java.net/~rpatil/8166138/webrev.02/
> <http://cr.openjdk.java.net/%7Erpatil/8166138/webrev.02/>
>
> Thanks,
>
> Pallavi Sonal.
>
> -----Original Message-----
>
> From: Stephen Colebourne<scolebourne at joda.org>
> <mailto:scolebourne at joda.org>
>
> Sent: Thursday, September 20, 2018 6:38 PM
>
> To: core-libs-dev<core-libs-dev at openjdk.java.net>
> <mailto:core-libs-dev at openjdk.java.net>
>
> Subject: Re: RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT
>
> should handle offsets
>
> Thanks for the update.
>
> The test case does not cover the situation of MAX/MIN instant and an offset other than zero, where the offset makes the instant invalid.
>
> eg. a negative offset at MAX or a positive offset at MIN.
>
> The doc of appendInstant() in DateTimeFormatterBuilder should be clarified to cover the fact that any OffsetId is parsed.
>
> thanks
>
> Stephen
>
> On Thu, 20 Sep 2018 at 13:54, Pallavi Sonal<pallavi.sonal at oracle.com>
> <mailto:pallavi.sonal at oracle.com> wrote:
>
> Thanks Roger , Naoto and Stephen for the review and valuable inputs.
>
> Here is the updated webrev for review :
>
> http://cr.openjdk.java.net/~rpatil/8166138/webrev.01/
> <http://cr.openjdk.java.net/%7Erpatil/8166138/webrev.01/>
>
> Thanks,
>
> Pallavi Sonal
>
> -----Original Message-----
>
> From: Stephen Colebourne<scolebourne at joda.org>
> <mailto:scolebourne at joda.org>
>
> Sent: Thursday, September 20, 2018 2:50 AM
>
> To: core-libs-dev<core-libs-dev at openjdk.java.net>
> <mailto:core-libs-dev at openjdk.java.net>
>
> Subject: Re: RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT
>
> should handle offsets
>
> Thanks for looking at this.
>
> The proposed fix does not tackle the bug fully. The bug is that
>
> the spec says
>
> "The format consists of: The ISO_OFFSET_DATE_TIME where ..."
>
> As such, the format must parse *any* offset, not just "Z" / "+00:00" etc.
>
> In addition, the fix doesn't work properly. Parsers work off a CharSequence which may be much longer than the instant. For example, the instant might be followed by a literal space and then a ZoneId.
>
> Using (length - 3) is simply not a valid approach - the parsing code cannot use the length like that.
>
> Furthermore, although there are numerous valid ISO-8601 ways of
>
> saying zero, this format uses dashes and colons in the date/time
>
> part, so
>
> ISO-8601 restricts the offset to only those formats that include colons.
>
> I think it simply needs the appendLiteral(Z) changing to appendOffsetId() And line 3495 changes to use the offset from the newContext.
>
> thanks
>
> Stephen
>
> On Wed, 19 Sep 2018 at 18:16, Pallavi Sonal<pallavi.sonal at oracle.com>
> <mailto:pallavi.sonal at oracle.com> wrote:
>
> Hi,
>
> Please review the changes to the following issue:
>
> Bug :https://bugs.openjdk.java.net/browse/JDK-8166138
>
> The proposed fix is located at:
>
> Webrev :http://cr.openjdk.java.net/~rpatil/8166138/webrev.00/
> <http://cr.openjdk.java.net/%7Erpatil/8166138/webrev.00/>
>
> As per ISO 8601 standards, an offset of zero, in addition to having the special representation "Z", can also be stated numerically as "+00:00", "+0000", or "+00" [1]. With this fix, Instant.parse() can parse a String containing the zero offsets in any of these three forms. Any other offset apart from "Z", "+00:00", "+0000", or "+00" will not be accepted in the input string to be parsed.
>
> [1]https://en.wikipedia.org/wiki/ISO_8601
>
> Thanks,
>
> Pallavi Sonal
>
> --
>
> Thanks, Roger
>
>
>
> --
>
> Thanks, Roger
>
More information about the core-libs-dev
mailing list