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.so...@oracle.com>; Roger Riggs <roger.ri...@oracle.com>; core-libs-dev <core-libs-dev@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.so...@oracle.com>; core-libs-dev > <core-libs-dev@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/ >> >> 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 >> >
