This is all about fixing a messy API that was created in 8. The methods propose are all about consistency. The toSeconds() method completes the set. For each unit there is a toXxx() and a toXxxPart(). Missing one out makes everything worse.
Stephen On 30 November 2015 at 19:02, Roger Riggs <roger.ri...@oracle.com> wrote: > Hi Stephen, Nadeesh, > > The toXXXPart methods look fine. > > I'm not entirely convinced that the toSeconds() method is worth it and may > be > deemed as confusing with the new toSecondsPart method. > How many people have problems with getSeconds()? > > The toXXXPart tests could have used a single DataProvider with > the values supplied for each unit to reduce the duplication. > > Thanks, Roger > > > > > > On 11/30/2015 09:29 AM, Stephen Colebourne wrote: >> >> I think thats ready to be merged, thanks >> Stephen >> >> On 30 November 2015 at 11:26, nadeesh tv <nadeesh...@oracle.com> wrote: >>> >>> Hi all, >>> Please see the updated webrev >>> http://cr.openjdk.java.net/~ntv/8142936/webrev.02/ >>> Regards, >>> Nadeesh TV >>> >>> On 11/27/2015 11:20 PM, Stephen Colebourne wrote: >>>> >>>> "Overall, looks pretty good. >>>> >>>> There a a number of double spaces that should be single spaces in the >>>> Javadoc. >>>> >>>> Change >>>> "This is based on the standard definition of a day has 24 hours." >>>> to >>>> "This is based on the standard definition of a day as 24 hours." >>>> ("has" to "as") >>>> There are three places to fix this. >>>> >>>> The toMillisPart() docs could do with some rework. >>>> change >>>> "number of milliseconds part in the nanosecond part of the duration" >>>> to >>>> "number of milliseconds part of the duration" >>>> >>>> try this: >>>> "This returns the milliseconds part by dividing the number of >>>> nanoseconds by 1,000,000." >>>> >>>> On the tests. >>>> >>>> There is no test for toSeconds(). >>>> >>>> For the other tests, I have been bitten before by not testing edge >>>> cases. >>>> A test for zero, and for a negative round number would be good. >>>> eg. for toHoursPart() the round number would be a duration of exactly >>>> minus 2 hours. >>>> >>>> Duration.ofHours(2).toDaysPart() = 0 >>>> Duration.ofHours(2).toHoursPart() = 2 >>>> Duration.ofHours(2).toMinutesPart() = 0 >>>> Duration.ofHours(2).toSecondsPart() = 0 >>>> Duration.ofHours(2).toMillisPart() = 0 >>>> Duration.ofHours(2).toNanosPart() = 0 >>>> >>>> thus really six tests are needed each time. The best way to achieve >>>> this is using @DataProvider in TestNG, where you can setup a data grid >>>> of inputs and 6 expected outputs. >>>> >>>> thanks >>>> Stephen >>>> >>>> >>>> >>>> >>>> On 26 November 2015 at 06:41, nadeesh tv <nadeesh...@oracle.com> wrote: >>>>> >>>>> Hi all, >>>>> >>>>> Please review a fix for >>>>> >>>>> Bug Id - https://bugs.openjdk.java.net/browse/JDK-8142936 >>>>> >>>>> -Enhance Duration by adding toNanosPart() , >>>>> >>>>> toMillisPart(),toSecondsPart(),toMinutesPart(),toHoursPart(),toDaysPart() >>>>> methods . >>>>> - Had to rename private BigDecimal toSeconds() -> private >>>>> BigDecimal >>>>> toBigDecimalSeconds() >>>>> >>>>> >>>>> webrev - http://cr.openjdk.java.net/~ntv/8142936/webrev.01/ >>>>> >>>>> -- >>>>> Thanks and Regards, >>>>> Nadeesh TV >>>>> <div id="DDB4FAA8-2DD7-40BB-A1B8-4E2AA1F9FDF2"><table >>>>> style="border-top: >>>>> 1px solid #aaabb6; margin-top: 10px;"> >>>> >>>> <tr> >>>> <td style="width: 105px; padding-top: 15px;"> >>>> <a >>>> >>>> href="https://www.avast.com/?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail"; >>>> target="_blank"><img >>>> src="https://ipmcdn.avast.com/images/logo-avast-v1.png"; style="width: >>>> 90px; height:33px;"/></a> >>>> </td> >>>> <td style="width: 470px; padding-top: 20px; color: >>>> #41424e; >>>> font-size: 13px; font-family: Arial, Helvetica, sans-serif; >>>> line-height: 18px;">This email has been sent from a virus-free >>>> computer protected by Avast. <br /><a >>>> >>>> >>>> href="https://www.avast.com/?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail"; >>>> target="_blank" style="color: #4453ea;">www.avast.com</a> >>>> </td> >>>> </tr> >>>> </table><a href="#DDB4FAA8-2DD7-40BB-A1B8-4E2AA1F9FDF2" width="1" >>>> height="1"></a></div> >>> >>> >>> -- >>> Thanks and Regards, >>> Nadeesh TV >>> >
![https://ipmcdn.avast.com/images/logo-avast-v1.png"](https://ipmcdn.avast.com/images/logo-avast-v1.png"){kind=link}