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

Roger Riggs Roger.Riggs at oracle.com
Mon Oct 1 15:59:17 UTC 2018 

Hi,

The checkbox for compatibility Kind Behavioral should be checked. (You 
may need to hit Edit to see it).
A simple description of the compatibility risk may help clarify minimal.
Something to the effect, that there is no change in formatting behavior
and that parsing will now accept a numeric offset.

Otherwise, looks fine.

Thanks, Roger

On 10/01/2018 09:24 AM, Pallavi Sonal wrote:
> 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.sonal at oracle.com>; Roger Riggs <roger.riggs at oracle.com>; core-libs-dev <core-libs-dev at 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.sonal at oracle.com>; core-libs-dev
>> <core-libs-dev at 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 at 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.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
>>>

-- 
Thanks, Roger

More information about the core-libs-dev mailing list