Hi Roger,
Please see the answers inline
On 5/3/2016 2:43 AM, Roger Riggs wrote:
Hi Nadeesh,
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
line 1835: the appendValue(field) method has a sign-style of NORMAL;
should that be NOT_NEGATIVE?
It's 'NOT_NEGATIVE' only. May be a browser caching issue. Can you
please check it once again.
test/java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java:
What is being tested in test_dayOfYearFieldPattern?
It does not check that the value parsed matches the input.
These test cases added initially. Now it's become almost redundant due
to 'dayOfYearFieldValues' . I could remove it.
test/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java
686, 688: should these cases show with sign-style NOT_NEGATIVE also?
Suspecting browser caching here also.
Updated the webrev by removing some unnecessary spaces
http://cr.openjdk.java.net/~ntv/8079628/webrev.03/
Regards,
Nadeesh TV
Thanks, Roger
On 4/28/2016 2:04 PM, nadeesh tv wrote:
Hi all,
Please see the updated webrev
http://cr.openjdk.java.net/~ntv/8079628/webrev.02/
Thanks and Regards,
Nadeesh TV
On 4/28/2016 8:17 PM, Stephen Colebourne wrote:
My mistake on the spec for "DD". It should be SignStyle.NOT_NEGATIVE,
not NORMAL.
I'd like to see a test that checks adjacent value parsing works
correctly for "DDD". ie. yyyyDDDHHmm should be able to parse into a
LocalDateTime where the date-time value can be checked against the
input string.
I think this will be a good fix once complete.
thanks
Stephen
On 28 April 2016 at 14:10, nadeesh tv <nadeesh...@oracle.com> wrote:
Hi all,
Thanks Stephen for the comments.
Please see the updated webrev
http://cr.openjdk.java.net/~ntv/8079628/webrev.01/
Regards,
Nadeesh TV
On 4/26/2016 5:42 PM, Stephen Colebourne wrote:
This change introduces inconsistencies and problems. For example, it
will parse up to 19 digits for "D" but only up to 2 digits for "d".
The following would be better:
* D 1 appendValue(ChronoField.DAY_OF_YEAR)
* DD 2 appendValue(ChronoField.DAY_OF_YEAR, 2, 3,
SignStyle.NORMAL)
* DDD 3 appendValue(ChronoField.DAY_OF_YEAR, 3)
This limits the accepted input to 3 digits, which is quite sufficient
here. It is also clearer for the common "D" and "DDD" cases.
Note that a test case needs to cover adjacent value parsing for
day-of-year. This pattern "yyyyDDD" should be tested and work. With
the webrev changes, it will not work as adjacent value parsing mode
will not be triggered.
(The change will alter the meaning of "yyyyDD" but no-one should be
using that anyway as it is broken, only working for the first 99 days
of the year.)
The code in TCKDateTimeFormatterBuilder needs a blank line after the
new methods.
Stephen
On 26 April 2016 at 12:48, nadeesh tv <nadeesh...@oracle.com> wrote:
Hi all,
Please review a fix for
Bug ID - https://bugs.openjdk.java.net/browse/JDK-8079628
Issue - Pattern 'D' is not implemented as intended by CLDR
Solution - Changed the definition of 'D' to D..D 1..3
appendValue(ChronoField.DAY_OF_YEAR, n, 19, SignStyle.NORMAL)
Webrev - http://cr.openjdk.java.net/~ntv/8079628/webrev.00/
--
Thanks and Regards,
Nadeesh TV
--
Thanks and Regards,
Nadeesh TV
--
Thanks and Regards,
Nadeesh TV
