Repository: moment/luxon (js)

Issue:
shiftAll() regression
**Describe the bug**
What used to be `P99Y11M3W2DT10H29M6S` in v3.3.0 is now `P99Y11M3.3481250000000005W` in v3.4.2

**To Reproduce**
```ts
const { DateTime, Duration } = require("luxon@3.4.2");

const purchaseDate = DateTime.fromObject(
  { day: 1, month: 1, year: 2020 },
  { zone: "UTC", conversionAccuracy: "longterm" }
);
const warranty = Duration.fromObject({ years: 100 }, { zone: "UTC", conversionAccuracy: "longterm" });
const now = DateTime.fromObject(
  { day: 8, month: 1, year: 2020 },
  { zone: "UTC", conversionAccuracy: "longterm" }
);
const age = now.diff(purchaseDate).shiftToAll();
const warrantyLeft = warranty.minus(age).shiftToAll();

console.log({
  warrantyLeft: warrantyLeft.toString(),
});
```
You can run it on https://npm.runkit.com/luxon and see the unexpected `{warrantyLeft: "P99Y11M3.3481250000000005W"}` in the output.

**Actual vs Expected behavior**
```ts
const { DateTime, Duration } = require("luxon@3.3.0");

const purchaseDate = DateTime.fromObject(
  { day: 1, month: 1, year: 2020 },
  { zone: "UTC", conversionAccuracy: "longterm" }
);
const warranty = Duration.fromObject({ years: 100 }, { zone: "UTC", conversionAccuracy: "longterm" });
const now = DateTime.fromObject(
  { day: 8, month: 1, year: 2020 },
  { zone: "UTC", conversionAccuracy: "longterm" }
);
const age = now.diff(purchaseDate).shiftToAll();
const warrantyLeft = warranty.minus(age).shiftToAll();

console.log({
  warrantyLeft: warrantyLeft.toString(),
});
```
You can run it on https://npm.runkit.com/luxon and see the expected `{warrantyLeft: "P99Y11M3W2DT10H29M6S"}` in the output.

**Desktop (please complete the following information):**

- OS: WSL2 in Windows
- Browser N/A
- Luxon version 3.4.2
- Your timezone Australia/Sydney

**Additional context**
Might be loosely related to https://github.com/moment/luxon/issues/1482 ?

Pull request:
Fix Duration#normalize, shiftTo and shiftToAll
This fixes #1496 by introducing an additional conversion step in `Duration#normalize` / `normalizeValues`.
This step converts any fractions in larger units into lower units, if possible. 
For example: `{ years: 2.5, days: 0, hours: 0 }` becomes `{ years: 2, days: 182, hours: 12 }` (assuming casual conversions).

This also affects `shiftTo` and `shiftToAll`. I have added tests for all three methods to test this behavior.

Note: This alters the behavior of `normalize`, because previously `normalize` would not do this. `{years: 2.5, days: 0}` would stay as-is. I would argue that that is a bug however, which is fixed by this pull request. `shiftTo` (and by extension `shiftToAll`) _did_ do this, prior to calling `normalize`, however I would argue that this is something that should be handled by `normalize`.

Patch:
diff --git a/src/duration.js b/src/duration.js
index b725b59e..280c4c98 100644
--- a/src/duration.js
+++ b/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 patch:
diff --git a/test/duration/units.test.js b/test/duration/units.test.js
index 42aaf239..68cd57af 100644
--- a/test/duration/units.test.js
+++ b/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()
 //-------