The test test_strict_offset_adjacentValidPattern_parse should also check that the hour of day was parsed to 12.
I don't think there are any tests of the lenient behaviour? thanks Stephen On 10 June 2016 at 12:17, nadeesh tv <nadeesh...@oracle.com> wrote: > Hi, > > Please see the updated webrev > http://cr.openjdk.java.net/~ntv/8066806/webrev.08/ > > Thanks and Regards, > Nadeesh > > On 6/9/2016 4:29 PM, nadeesh tv wrote: >> >> Hi Stephen, >> >> On 6/9/2016 4:19 PM, Stephen Colebourne wrote: >>> >>> "absHours / 10 > 0" would be simpler as "absHours >= 10" >>> >>> Around line 3595 we have >>> boolean paddedHour = isPaddedHour(); >>> but 6 lines down isPaddedHour() is used, not the local variable >>> >>> There is an extra space in the message here: >>> new DateTimeException(" Value out of Range >>> also, I'd use "range", not "Range". >> >> Thanks. >>> >>> >>> The maximum for ZoneOffset is actually 18:00:00 not 23:59:59, however >>> it is not worth validating that here. The base validation for 23/59/59 >>> that has been added is just fine, values between 18 and 23 will be >>> rejected later in the processs when attempting to create an instance >>> of ZoneOffset. >>> >>> I don't see any tests for >>> >>> new DateTimeFormatterBuilder().appendOffset(pattern, >>> "Z").appendValue(ChronoField.NANO_OF_DAY).toFormatter().parse(offset + >>> "12345") >>> >>> which should work for most of the patterns. >> >> I intentionally avoided it. I will create a positive test cases for >> working patterns and negative test cases for rest. >> >> Regards, >> Nadeesh >>> >>> >>> thanks >>> Stephen >>> >>> On 9 June 2016 at 10:27, nadeesh tv <nadeesh...@oracle.com> wrote: >>>> >>>> Hi Roger, >>>> Thanks for the comments. >>>> >>>> Please see the updated webrev >>>> http://cr.openjdk.java.net/~ntv/8066806/webrev.07/ >>>> Thanks and regards, >>>> Nadeesh >>>> >>>> On 6/9/2016 2:36 AM, Roger Riggs wrote: >>>> >>>> HI Nadeesh, >>>> >>>> Looking better >>>> >>>> DateTimeFormatterBuilder: >>>> >>>> - line 3678: If array[1] == 24, offsetSeconds will be greater that >>>> seconds >>>> in a day; that's not right. >>>> I don't think hour=24 is valid. (and there would be test case(s) >>>> for >>>> it.) >>>> >>>> There should be test cases for offsets over the limit of hours, minutes, >>>> and >>>> seconds: 24:60:60 >>>> >>>> Thanks, Roger >>>> >>>> >>>> >>>> On 6/8/2016 2:59 AM, nadeesh tv wrote: >>>> >>>> Hi, >>>> >>>> Please see the updated webrev >>>> http://cr.openjdk.java.net/~ntv/8066806/webrev.06/ >>>> >>>> I reused code provided by Stephen and handled the edge cases accordingly >>>> >>>> Thanks and Regards, >>>> Nadeesh >>>> >>>> On 5/31/2016 7:15 PM, Stephen Colebourne wrote: >>>> >>>> Where the new patterns are described in Javadoc, there is no >>>> discussion of the difference between "H" and "HH". >>>> >>>> Add after </ul> >>>> >>>> "Patterns containing "HH" will format and parse a two digit hour, >>>> zero-padded if necessary. Patterns containing "H" will format with no >>>> zero-padding, and parse either one or two digits." >>>> >>>> "with colo" should be "with colon" >>>> >>>> As for the main code, I've had a go at a rewrite: >>>> https://gist.github.com/jodastephen/68857dd344e33bd6c0b3b4d24279d2e4 >>>> >>>> It is completely untested, and surely has mistakes, however as a >>>> design it seems reasonable. >>>> >>>> I agree that the tests need to cover these cases: >>>> >>>> - offset at end of line >>>> - offset followed by letters >>>> - offset followed by numbers >>>> >>>> Stephen >>>> >>>> >>>> On 26 May 2016 at 08:49, nadeesh tv <nadeesh...@oracle.com> wrote: >>>> >>>> Hi all, >>>> >>>> Please review >>>> >>>> BugId : https://bugs.openjdk.java.net/browse/JDK-8066806 >>>> >>>> Issue: java.time.format.DateTimeFormatter cannot parse an offset with >>>> single >>>> digit hour >>>> >>>> webrev: http://cr.openjdk.java.net/~ntv/8066806/webrev.03/ >>>> >>>> Solution: Added the suggested patterns but the parsing logic became too >>>> complex. >>>> Appreciate any suggestion to make the parsing less complicated >>>> >>>> -- >>>> Thanks and Regards, >>>> Nadeesh TV >>>> >>>> >>>> >>>> >>>> -- >>>> Thanks and Regards, >>>> Nadeesh TV >>>> >> > > -- > Thanks and Regards, > Nadeesh TV >
