Commons Lang
  1. Commons Lang
  2. LANG-282

Create more tests to test out the +=31 replacement code in DurationFormatUtils.

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.3
    • Component/s: None
    • Labels:
      None

      Description

      Code being:

      while (days < 0)

      { end.add(Calendar.MONTH, -1); days += end.getActualMaximum(Calendar.DAY_OF_MONTH); //days += 31; // TODO: Need tests to show this is bad and the new code is good. // HEN: It's a tricky subject. Jan 15th to March 10th. If I count days-first it is // 1 month and 26 days, but if I count month-first then it is 1 month and 23 days. // Also it's contextual - if asked for no M in the format then I should probably // be doing no calculating here. months -= 1; end.add(Calendar.MONTH, 1); }

        Activity

        Mark Thomas made changes -
        Workflow jira [ 12386447 ] Default workflow, editable Closed status [ 12602128 ]
        Henri Yandell made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Closed [ 6 ]
        Hide
        Henri Yandell added a comment -

        This has (I think) now been tested to death (last commit on this subject is r488926, more tests and bugfixes/rewrite).

        I'm sure there are still bugs hiding away in here, but it's many leagues ahead of the version in 2.2.

        Show
        Henri Yandell added a comment - This has (I think) now been tested to death (last commit on this subject is r488926, more tests and bugfixes/rewrite). I'm sure there are still bugs hiding away in here, but it's many leagues ahead of the version in 2.2.
        Hide
        Henri Yandell added a comment -

        More tests added and bugfixes - including a switch from the 1m23d choice to the 1m26d choice as this balances more nicely with the code to work out the actual number of days between the two.

        The actual code still chooses Stephen's third answer of 2m-5d as it always has; however it then attempts to make that more readable. Another way to look at this choice other than the days-first/months-first concept is "which month do you take the -5 away from, the first or the penultimate?". The code just changed from removing 5 from the penultimate to removing 5 from the first.

        See r485481.

        Show
        Henri Yandell added a comment - More tests added and bugfixes - including a switch from the 1m23d choice to the 1m26d choice as this balances more nicely with the code to work out the actual number of days between the two. The actual code still chooses Stephen's third answer of 2m-5d as it always has; however it then attempts to make that more readable. Another way to look at this choice other than the days-first/months-first concept is "which month do you take the -5 away from, the first or the penultimate?". The code just changed from removing 5 from the penultimate to removing 5 from the first. See r485481.
        Hide
        Stephen Colebourne added a comment -

        In fact, there is a third answer to the question 2 months and -5 days.

        Show
        Stephen Colebourne added a comment - In fact, there is a third answer to the question 2 months and -5 days.
        Hide
        Henri Yandell added a comment -

        The following causes an error in DurationFormatUtilsTest.testEdgeDurations.

        + assertEqualDuration( "54", new int[]

        { 2006, 0, 15, 0, 0, 0 }

        ,
        + new int[]

        { 2006, 2, 10, 0, 0, 0 }

        , "dd");

        It is bizarrely returning 77.

        77 - 10 - 16 - 28 = 23. (ie: - Mar - Jan, - Feb)

        Interesting that that is the number of days found when calculating "MM dd".

        Show
        Henri Yandell added a comment - The following causes an error in DurationFormatUtilsTest.testEdgeDurations. + assertEqualDuration( "54", new int[] { 2006, 0, 15, 0, 0, 0 } , + new int[] { 2006, 2, 10, 0, 0, 0 } , "dd"); It is bizarrely returning 77. 77 - 10 - 16 - 28 = 23. (ie: - Mar - Jan, - Feb) Interesting that that is the number of days found when calculating "MM dd".
        Hide
        Henri Yandell added a comment -

        Going with the 1 month 23 day choice as the code currently does that - with a recommendation of Joda Time if more control is needed.

        svn ci -m "Added javadoc to explain the quandry in how to count month/day differences" src/java/
        Sending src/java/org/apache/commons/lang/time/DurationFormatUtils.java
        Transmitting file data .
        Committed revision 483891.

        Show
        Henri Yandell added a comment - Going with the 1 month 23 day choice as the code currently does that - with a recommendation of Joda Time if more control is needed. svn ci -m "Added javadoc to explain the quandry in how to count month/day differences" src/java/ Sending src/java/org/apache/commons/lang/time/DurationFormatUtils.java Transmitting file data . Committed revision 483891.
        Henri Yandell made changes -
        Fix Version/s 2.3 [ 12311948 ]
        Fix Version/s 3.0 [ 12311714 ]
        Hide
        Stephen Colebourne added a comment -

        As far as Joda-Time is concerned, both answers to the question are right.

        Show
        Stephen Colebourne added a comment - As far as Joda-Time is concerned, both answers to the question are right.
        Henri Yandell made changes -
        Field Original Value New Value
        Fix Version/s 2.3 [ 12311948 ]
        Fix Version/s 3.0 [ 12311714 ]
        Henri Yandell created issue -

          People

          • Assignee:
            Henri Yandell
            Reporter:
            Henri Yandell
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development