From e3c4004c9ac4ad13ad33963524d99d33196d5ffb Mon Sep 17 00:00:00 2001 From: Take Weiland Date: Tue, 5 Sep 2023 20:29:57 +0200 Subject: [PATCH] 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 --- src/duration.js | 21 +++++++++++-- test/duration/units.test.js | 61 +++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) 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() { 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() //-------