From ca88eea9f3aec3117e78f1817322dc7b41949e22 Mon Sep 17 00:00:00 2001 From: Take Weiland Date: Sun, 3 Sep 2023 16:36:55 +0200 Subject: [PATCH 1/5] Fix normalizeValues not converting fractional units into lower order units --- src/duration.js | 69 +++++++++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 28 deletions(-) diff --git a/src/duration.js b/src/duration.js index b725b59e7..fc3543b8e 100644 --- a/src/duration.js +++ b/src/duration.js @@ -141,36 +141,49 @@ function normalizeValues(matrix, vals) { // the logic below assumes the overall value of the duration is positive // 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) => { - if (!isUndefined(vals[current])) { - if (previous) { - const previousVal = vals[previous] * factor; - const conv = matrix[current][previous]; - - // if (previousVal < 0): - // lower order unit is negative (e.g. { years: 2, days: -2 }) - // normalize this by reducing the higher order unit by the appropriate amount - // and increasing the lower order unit - // this can never make the higher order unit negative, because this function only operates - // on positive durations, so the amount of time represented by the lower order unit cannot - // be larger than the higher order unit - // else: - // lower order unit is positive (e.g. { years: 2, days: 450 } or { years: -2, days: 450 }) - // in this case we attempt to convert as much as possible from the lower order unit into - // the higher order one - // - // Math.floor takes care of both of these cases, rounding away from 0 - // if previousVal < 0 it makes the absolute value larger - // if previousVal >= it makes the absolute value smaller - const rollUp = Math.floor(previousVal / conv); - vals[current] += rollUp * factor; - vals[previous] -= rollUp * conv * factor; + const presentUnits = reverseUnits.filter((u) => !isUndefined(vals[u])); + + presentUnits.reduce((previous, current, currentIndex) => { + if (previous) { + const previousVal = vals[previous] * factor; + const conv = matrix[current][previous]; + + // if (previousVal < 0): + // lower order unit is negative (e.g. { years: 2, days: -2 }) + // normalize this by reducing the higher order unit by the appropriate amount + // and increasing the lower order unit + // this can never make the higher order unit negative, because this function only operates + // on positive durations, so the amount of time represented by the lower order unit cannot + // be larger than the higher order unit + // else: + // lower order unit is positive (e.g. { years: 2, days: 450 } or { years: -2, days: 450 }) + // in this case we attempt to convert as much as possible from the lower order unit into + // the higher order one + // + // Math.floor takes care of both of these cases, rounding away from 0 + // if previousVal < 0 it makes the absolute value larger + // if previousVal >= it makes the absolute value smaller + const rollUp = Math.floor(previousVal / conv); + vals[current] += rollUp * factor; + vals[previous] -= rollUp * conv * factor; + // try to convert any decimals into units smaller than "previous" + // this happens especially with conversionAccuracy: longterm + // for example for { years: 2, days: 450, seconds: 0 } we get { years: 3, days: 84.7575, seconds: 0 } at first + // we want to convert the fractional days into the lower order unit seconds + for (let i = currentIndex - 2; i >= 0; i--) { + let fraction = vals[previous] % 1; + if (fraction === 0) break; + const lowerOrderUnit = presentUnits[i]; + const lowerOrderConv = matrix[previous][lowerOrderUnit]; + let rollDown = fraction * lowerOrderConv; + if (i !== 0) { + rollDown = Math.floor(rollDown); + } + vals[lowerOrderUnit] += rollDown; + vals[previous] -= rollDown / lowerOrderConv; } - return current; - } else { - return previous; } + return current; }, null); } From b6cc095cfd82ee69b3f4495bd610c0e8ea443298 Mon Sep 17 00:00:00 2001 From: Take Weiland Date: Sun, 3 Sep 2023 18:00:13 +0200 Subject: [PATCH 2/5] Clarify Duration#normalize with fractional parts --- src/duration.js | 88 ++++++++++++++++++++++++++----------------------- 1 file changed, 46 insertions(+), 42 deletions(-) diff --git a/src/duration.js b/src/duration.js index fc3543b8e..7e04f1264 100644 --- a/src/duration.js +++ b/src/duration.js @@ -141,49 +141,51 @@ function normalizeValues(matrix, vals) { // the logic below assumes the overall value of the duration is positive // if this is not the case, factor is used to make it so const factor = durationToMillis(matrix, vals) < 0 ? -1 : 1; - const presentUnits = reverseUnits.filter((u) => !isUndefined(vals[u])); - - presentUnits.reduce((previous, current, currentIndex) => { - if (previous) { - const previousVal = vals[previous] * factor; - const conv = matrix[current][previous]; - - // if (previousVal < 0): - // lower order unit is negative (e.g. { years: 2, days: -2 }) - // normalize this by reducing the higher order unit by the appropriate amount - // and increasing the lower order unit - // this can never make the higher order unit negative, because this function only operates - // on positive durations, so the amount of time represented by the lower order unit cannot - // be larger than the higher order unit - // else: - // lower order unit is positive (e.g. { years: 2, days: 450 } or { years: -2, days: 450 }) - // in this case we attempt to convert as much as possible from the lower order unit into - // the higher order one - // - // Math.floor takes care of both of these cases, rounding away from 0 - // if previousVal < 0 it makes the absolute value larger - // if previousVal >= it makes the absolute value smaller - const rollUp = Math.floor(previousVal / conv); - vals[current] += rollUp * factor; - vals[previous] -= rollUp * conv * factor; - // try to convert any decimals into units smaller than "previous" - // this happens especially with conversionAccuracy: longterm - // for example for { years: 2, days: 450, seconds: 0 } we get { years: 3, days: 84.7575, seconds: 0 } at first - // we want to convert the fractional days into the lower order unit seconds - for (let i = currentIndex - 2; i >= 0; i--) { - let fraction = vals[previous] % 1; - if (fraction === 0) break; - const lowerOrderUnit = presentUnits[i]; - const lowerOrderConv = matrix[previous][lowerOrderUnit]; - let rollDown = fraction * lowerOrderConv; - if (i !== 0) { - rollDown = Math.floor(rollDown); - } - vals[lowerOrderUnit] += rollDown; - vals[previous] -= rollDown / lowerOrderConv; + + orderedUnits.reduceRight((previous, current) => { + if (!isUndefined(vals[current])) { + if (previous) { + const previousVal = vals[previous] * factor; + const conv = matrix[current][previous]; + + // if (previousVal < 0): + // lower order unit is negative (e.g. { years: 2, days: -2 }) + // normalize this by reducing the higher order unit by the appropriate amount + // and increasing the lower order unit + // this can never make the higher order unit negative, because this function only operates + // on positive durations, so the amount of time represented by the lower order unit cannot + // be larger than the higher order unit + // else: + // lower order unit is positive (e.g. { years: 2, days: 450 } or { years: -2, days: 450 }) + // in this case we attempt to convert as much as possible from the lower order unit into + // the higher order one + // + // Math.floor takes care of both of these cases, rounding away from 0 + // if previousVal < 0 it makes the absolute value larger + // if previousVal >= it makes the absolute value smaller + const rollUp = Math.floor(previousVal / conv); + vals[current] += rollUp * factor; + vals[previous] -= rollUp * conv * factor; + } + return current; + } else { + 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; } - return current; }, null); } @@ -723,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 }) * @return {Duration} */ normalize() { From c638a71381c2930f388c7fda00c7df198c3e3c1c Mon Sep 17 00:00:00 2001 From: Take Weiland Date: Sun, 3 Sep 2023 18:16:13 +0200 Subject: [PATCH 3/5] Add tests for new Duration#normalize behavior --- test/duration/units.test.js | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/duration/units.test.js b/test/duration/units.test.js index 42aaf2397..3b3e56685 100644 --- a/test/duration/units.test.js +++ b/test/duration/units.test.js @@ -243,6 +243,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() //------- From 464989240ae4b7d4c130795045c7b800f80bfc18 Mon Sep 17 00:00:00 2001 From: Take Weiland Date: Sun, 3 Sep 2023 18:33:50 +0200 Subject: [PATCH 4/5] Add tests for shiftTo and shiftToAll only producing fractions in the lowest unit --- test/duration/units.test.js | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/duration/units.test.js b/test/duration/units.test.js index 3b3e56685..68cd57af6 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); From 851444634a414b1669fa5a311327c9d2ad2edebc Mon Sep 17 00:00:00 2001 From: Take Weiland Date: Sun, 3 Sep 2023 18:38:21 +0200 Subject: [PATCH 5/5] Finish Duration#normalize fourth example --- src/duration.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/duration.js b/src/duration.js index 7e04f1264..280c4c984 100644 --- a/src/duration.js +++ b/src/duration.js @@ -734,7 +734,7 @@ export default class Duration { * @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 }) + * @example Duration.fromObject({ years: 2.5, days: 0, hours: 0 }).normalize().toObject() //=> { years: 2, days: 182, hours: 12 } * @return {Duration} */ normalize() {