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 >
