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

Reply via email to