RFR:JDK-8079628:java.time: DateTimeFormatter containing "DD" fails on 3-digit day-of-year value

[nadeesh tv]

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.tv at 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.tv at 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