Fix Duration#normalize, shiftTo and shiftToAll (#1498)

* Fix normalizeValues not converting fractional units into lower order units

* Clarify Duration#normalize with fractional parts

* Add tests for new Duration#normalize behavior

* Add tests for shiftTo and shiftToAll only producing fractions in the lowest unit

* Finish Duration#normalize fourth example

Author: diesieben07

src/duration.js

@@ -142,7 +142,7 @@ function normalizeValues(matrix, vals) {
   // if this is not the case, factor is used to make it so
   const factor = durationToMillis(matrix, vals) < 0 ? -1 : 1;
 
-  reverseUnits.reduce((previous, current) => {
+  orderedUnits.reduceRight((previous, current) => {
     if (!isUndefined(vals[current])) {
       if (previous) {
         const previousVal = vals[previous] * factor;
@@ -172,6 +172,21 @@ function normalizeValues(matrix, vals) {
       return previous;
     }
   }, null);
+
+  // try to convert any decimals into smaller units if possible
+  // for example for { years: 2.5, days: 0, seconds: 0 } we want to get { years: 2, days: 182, hours: 12 }
+  orderedUnits.reduce((previous, current) => {
+    if (!isUndefined(vals[current])) {
+      if (previous) {
+        const fraction = vals[previous] % 1;
+        vals[previous] -= fraction;
+        vals[current] += fraction * matrix[previous][current];
+      }
+      return current;
+    } else {
+      return previous;
+    }
+  }, null);
 }
 
 // Remove all properties with a value of 0 from an object
@@ -710,14 +725,16 @@ export default class Duration {
   /**
    * Reduce this Duration to its canonical representation in its current units.
    * Assuming the overall value of the Duration is positive, this means:
-   * - excessive values for lower-order units are converted to higher order units (if possible, see first and second example)
+   * - excessive values for lower-order units are converted to higher-order units (if possible, see first and second example)
    * - negative lower-order units are converted to higher order units (there must be such a higher order unit, otherwise
    *   the overall value would be negative, see second example)
+   * - fractional values for higher-order units are converted to lower-order units (if possible, see fourth example)
    *
    * If the overall value is negative, the result of this method is equivalent to `this.negate().normalize().negate()`.
    * @example Duration.fromObject({ years: 2, days: 5000 }).normalize().toObject() //=> { years: 15, days: 255 }
    * @example Duration.fromObject({ days: 5000 }).normalize().toObject() //=> { days: 5000 }
    * @example Duration.fromObject({ hours: 12, minutes: -45 }).normalize().toObject() //=> { hours: 11, minutes: 15 }
+   * @example Duration.fromObject({ years: 2.5, days: 0, hours: 0 }).normalize().toObject() //=> { years: 2, days: 182, hours: 12 }
    * @return {Duration}
    */
   normalize() {

test/duration/units.test.js

@@ -105,6 +105,17 @@ test("Duration#shiftTo handles mixed units", () => {
   });
 });
 
+test("Duration#shiftTo does not produce unnecessary fractions in higher order units", () => {
+  const duration = Duration.fromObject(
+    { years: 2.5, weeks: -1 },
+    { conversionAccuracy: "longterm" }
+  );
+  const shifted = duration.shiftTo("years", "weeks", "minutes").toObject();
+  expect(shifted.years).toBe(2);
+  expect(shifted.weeks).toBe(25);
+  expect(shifted.minutes).toBeCloseTo(894.6, 5);
+});
+
 //------
 // #shiftToAll()
 //-------
@@ -122,6 +133,22 @@ test("Duration#shiftToAll shifts to all available units", () => {
   });
 });
 
+test("Duration#shiftToAll does not produce unnecessary fractions in higher order units", () => {
+  const duration = Duration.fromObject(
+    { years: 2.5, weeks: -1, seconds: 0 },
+    { conversionAccuracy: "longterm" }
+  );
+  const toAll = duration.shiftToAll().toObject();
+  expect(toAll.years).toBe(2);
+  expect(toAll.months).toBe(5);
+  expect(toAll.weeks).toBe(3);
+  expect(toAll.days).toBe(2);
+  expect(toAll.hours).toBe(10);
+  expect(toAll.minutes).toBe(29);
+  expect(toAll.seconds).toBe(6);
+  expect(toAll.milliseconds).toBeCloseTo(0, 5);
+});
+
 test("Duration#shiftToAll maintains invalidity", () => {
   const dur = Duration.invalid("because").shiftToAll();
   expect(dur.isValid).toBe(false);
@@ -243,6 +270,40 @@ test("Duration#normalize can convert all unit pairs", () => {
   }
 });
 
+test("Duration#normalize moves fractions to lower-order units", () => {
+  expect(Duration.fromObject({ years: 2.5, days: 0, hours: 0 }).normalize().toObject()).toEqual({
+    years: 2,
+    days: 182,
+    hours: 12,
+  });
+  expect(Duration.fromObject({ years: -2.5, days: 0, hours: 0 }).normalize().toObject()).toEqual({
+    years: -2,
+    days: -182,
+    hours: -12,
+  });
+  expect(Duration.fromObject({ years: 2.5, days: 12, hours: 0 }).normalize().toObject()).toEqual({
+    years: 2,
+    days: 194,
+    hours: 12,
+  });
+  expect(Duration.fromObject({ years: 2.5, days: 12.25, hours: 0 }).normalize().toObject()).toEqual(
+    { years: 2, days: 194, hours: 18 }
+  );
+});
+
+test("Duration#normalize does not produce fractions in higher order units when rolling up negative lower order unit values", () => {
+  const normalized = Duration.fromObject(
+    { years: 100, months: 0, weeks: -1, days: 0 },
+    { conversionAccuracy: "longterm" }
+  )
+    .normalize()
+    .toObject();
+  expect(normalized.years).toBe(99);
+  expect(normalized.months).toBe(11);
+  expect(normalized.weeks).toBe(3);
+  expect(normalized.days).toBeCloseTo(2.436875, 7);
+});
+
 //------
 // #rescale()
 //-------