RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT should handle offsets

[Stephen Colebourne]

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> wrote:
>
> Thanks for the clarification. Here is the updated webrev for review :
> http://cr.openjdk.java.net/~rpatil/8166138/webrev.03/
>
> Thanks,
> Pallavi Sonal
>
> -----Original Message-----
> From: Stephen Colebourne <scolebourne at joda.org>
> Sent: Tuesday, September 25, 2018 4:39 PM
> To: core-libs-dev <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> 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>
> > Sent: Sunday, September 23, 2018 12:36 PM
> > To: core-libs-dev <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> 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/
> > >
> > > Thanks,
> > > Pallavi Sonal.
> > >
> > > -----Original Message-----
> > > From: Stephen Colebourne <scolebourne at joda.org>
> > > Sent: Thursday, September 20, 2018 6:38 PM
> > > To: core-libs-dev <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> 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/
> > > >
> > > > Thanks,
> > > > Pallavi Sonal
> > > >
> > > > -----Original Message-----
> > > > From: Stephen Colebourne <scolebourne at joda.org>
> > > > Sent: Thursday, September 20, 2018 2:50 AM
> > > > To: core-libs-dev <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> 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/
> > > > >
> > > > >
> > > > >
> > > > > 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
> > > > >
> > > > >
> > > > >
> > > > >