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 <[email protected]> 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 <[email protected]> > Sent: Tuesday, September 25, 2018 4:39 PM > To: core-libs-dev <[email protected]> > 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 <[email protected]> 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 <[email protected]> > > Sent: Sunday, September 23, 2018 12:36 PM > > To: core-libs-dev <[email protected]> > > 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 <[email protected]> > > 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 <[email protected]> > > > Sent: Thursday, September 20, 2018 6:38 PM > > > To: core-libs-dev <[email protected]> > > > 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 <[email protected]> > > > 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 <[email protected]> > > > > Sent: Thursday, September 20, 2018 2:50 AM > > > > To: core-libs-dev <[email protected]> > > > > 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 <[email protected]> > > > > 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 > > > > > > > > > > > > > > > > > > > >
