Thanks Roger and Stephen. Here is the updated webrev with the suggested changes: http://cr.openjdk.java.net/~rpatil/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@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.so...@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 <scolebou...@joda.org> >> Sent: Tuesday, September 25, 2018 4:39 PM >> To: core-libs-dev <core-libs-dev@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.so...@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 <scolebou...@joda.org> >>> Sent: Sunday, September 23, 2018 12:36 PM >>> To: core-libs-dev <core-libs-dev@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.so...@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 <scolebou...@joda.org> >>>> Sent: Thursday, September 20, 2018 6:38 PM >>>> To: core-libs-dev <core-libs-dev@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.so...@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 <scolebou...@joda.org> >>>>> Sent: Thursday, September 20, 2018 2:50 AM >>>>> To: core-libs-dev <core-libs-dev@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.so...@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 >>>>>> >>>>>> >>>>>> >>>>>> -- Thanks, Roger