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

[Roger Riggs]

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?

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.


test/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java

686, 688: should these cases show with sign-style NOT_NEGATIVE also?

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
>>>
>