RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT should handle offsets
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
Hi Pallavi, On 9/19/18 1:15 PM, Pallavi Sonal 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/ DateTimeFormatterBuilder.java: 3463+
It would be cleaner and not create any new objects to write a private method that compared a string with a subSequence. And the if statement would also be more readable. If the text CharSequence is shorter than 3 or 5 or 6, I think you'll see an unexpected index exception. That check could be built into the private method. Since the if ... then ... else identifies exactly what string is in the input, it could use only appendLiteral (instead of the pattern and appendOffset). Then only appendLiteral would be needed and there would be fewer variables and conditions in lines 3459-3470. 3478: the extra local variable isn't necessary if the (offsetPresent) ?... expression were used in-line. Thanks, Roger
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
Hi Pallavi, Thank you for working on this. I think the javadoc should also be updated, mentioning offset parsing. Naoto On 9/19/18 10:15 AM, Pallavi Sonal 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 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@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 , 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@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.sonal@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 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@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@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.sonal@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
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@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.sonal@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@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.sonal@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 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@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@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.sonal@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@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.sonal@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
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@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.sonal@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@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.sonal@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@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.sonal@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
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@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@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.sonal@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@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.sonal@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@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.sonal@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 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@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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
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@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@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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
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@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@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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 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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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
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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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. 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@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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
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@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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
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@oracle.com>; Roger Riggs <roger.riggs@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.sonal@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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
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@oracle.com>; Roger Riggs <roger.riggs@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.sonal@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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@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.sonal@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
Thanks Roger, made the suggested changes. Thanks, Pallavi Sonal From: Roger Riggs Sent: Monday, October 01, 2018 9:29 PM To: Pallavi Sonal <pallavi.sonal@oracle.com>; Naoto Sato <naoto.sato@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT should handle offsets 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 HYPERLINK "mailto:pallavi.sonal@oracle.com"<pallavi.sonal@oracle.com>; Roger Riggs HYPERLINK "mailto:roger.riggs@oracle.com"<roger.riggs@oracle.com>; core-libs-dev HYPERLINK "mailto:core-libs-dev@openjdk.java.net"<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 HYPERLINK "mailto:pallavi.sonal@oracle.com"<pallavi.sonal@oracle.com>; core-libs-dev HYPERLINK "mailto:core-libs-dev@openjdk.java.net"<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 HYPERLINK "mailto:core-libs-dev@openjdk.java.net"<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 HYPERLINK "mailto:pallavi.sonal@oracle.com"<pallavi.sonal@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 HYPERLINK "mailto:scolebourne@joda.org"<scolebourne@joda.org> Sent: Tuesday, September 25, 2018 4:39 PM To: core-libs-dev HYPERLINK "mailto:core-libs-dev@openjdk.java.net"<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 HYPERLINK "mailto:pallavi.sonal@oracle.com"<pallavi.sonal@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 HYPERLINK "mailto:scolebourne@joda.org"<scolebourne@joda.org> Sent: Sunday, September 23, 2018 12:36 PM To: core-libs-dev HYPERLINK "mailto:core-libs-dev@openjdk.java.net"<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 HYPERLINK "mailto:pallavi.sonal@oracle.com"<pallavi.sonal@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 HYPERLINK "mailto:scolebourne@joda.org"<scolebourne@joda.org> Sent: Thursday, September 20, 2018 6:38 PM To: core-libs-dev HYPERLINK "mailto:core-libs-dev@openjdk.java.net"<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 HYPERLINK "mailto:pallavi.sonal@oracle.com"<pallavi.sonal@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 HYPERLINK "mailto:scolebourne@joda.org"<scolebourne@joda.org> Sent: Thursday, September 20, 2018 2:50 AM To: core-libs-dev HYPERLINK "mailto:core-libs-dev@openjdk.java.net"<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 HYPERLINK "mailto:pallavi.sonal@oracle.com"<pallavi.sonal@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
+1. Added myself as a reviewer of the CSR. Thanks for working on this. Naoto On 10/1/18 9:38 AM, Pallavi Sonal wrote:
Thanks Roger, made the suggested changes.
Thanks,
Pallavi Sonal
*From:*Roger Riggs *Sent:* Monday, October 01, 2018 9:29 PM *To:* Pallavi Sonal <pallavi.sonal@oracle.com>; Naoto Sato <naoto.sato@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net> *Subject:* Re: RFR: JDK-8166138 - DateTimeFormatter.ISO_INSTANT should handle offsets
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@oracle.com> <mailto:pallavi.sonal@oracle.com>; Roger Riggs<roger.riggs@oracle.com> <mailto:roger.riggs@oracle.com>; core-libs-dev<core-libs-dev@openjdk.java.net> <mailto: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.sonal@oracle.com> <mailto:pallavi.sonal@oracle.com>; core-libs-dev
<core-libs-dev@openjdk.java.net> <mailto: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/ <http://cr.openjdk.java.net/%7Erpatil/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> <mailto: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.sonal@oracle.com> <mailto:pallavi.sonal@oracle.com> wrote:
Thanks for the clarification. Here is the updated webrev for review :
http://cr.openjdk.java.net/~rpatil/8166138/webrev.03/ <http://cr.openjdk.java.net/%7Erpatil/8166138/webrev.03/>
Thanks,
Pallavi Sonal
-----Original Message-----
From: Stephen Colebourne<scolebourne@joda.org> <mailto:scolebourne@joda.org>
Sent: Tuesday, September 25, 2018 4:39 PM
To: core-libs-dev<core-libs-dev@openjdk.java.net> <mailto: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.sonal@oracle.com> <mailto:pallavi.sonal@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@joda.org> <mailto:scolebourne@joda.org>
Sent: Sunday, September 23, 2018 12:36 PM
To: core-libs-dev<core-libs-dev@openjdk.java.net> <mailto: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.sonal@oracle.com> <mailto:pallavi.sonal@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/ <http://cr.openjdk.java.net/%7Erpatil/8166138/webrev.02/>
Thanks,
Pallavi Sonal.
-----Original Message-----
From: Stephen Colebourne<scolebourne@joda.org> <mailto:scolebourne@joda.org>
Sent: Thursday, September 20, 2018 6:38 PM
To: core-libs-dev<core-libs-dev@openjdk.java.net> <mailto: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.sonal@oracle.com> <mailto:pallavi.sonal@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/ <http://cr.openjdk.java.net/%7Erpatil/8166138/webrev.01/>
Thanks,
Pallavi Sonal
-----Original Message-----
From: Stephen Colebourne<scolebourne@joda.org> <mailto:scolebourne@joda.org>
Sent: Thursday, September 20, 2018 2:50 AM
To: core-libs-dev<core-libs-dev@openjdk.java.net> <mailto: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.sonal@oracle.com> <mailto:pallavi.sonal@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/ <http://cr.openjdk.java.net/%7Erpatil/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
participants (5)
-
Naoto Sato
-
naoto.sato＠oracle.com
-
Pallavi Sonal
-
Roger Riggs
-
Stephen Colebourne