RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT should handle offsets

[Roger Riggs]

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.sonal at 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 <scolebourne at joda.org>
>> Sent: Tuesday, September 25, 2018 4:39 PM
>> To: core-libs-dev <core-libs-dev at 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.sonal at 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 <scolebourne at joda.org>
>>> Sent: Sunday, September 23, 2018 12:36 PM
>>> To: core-libs-dev <core-libs-dev at 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.sonal at 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 <scolebourne at joda.org>
>>>> Sent: Thursday, September 20, 2018 6:38 PM
>>>> To: core-libs-dev <core-libs-dev at 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.sonal at 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 <scolebourne at joda.org>
>>>>> Sent: Thursday, September 20, 2018 2:50 AM
>>>>> To: core-libs-dev <core-libs-dev at 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.sonal at 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