Closed whitelynx closed 6 years ago
The problem with the failing tests seems to specifically show up with { round: Math.ceil }
, and specifically when the time is between 11 months, 4 weeks
and the next year (regardless of how many years are present) -- for some reason, in that range, it renders 13 months
instead of just rounding up to the next year like it does immediately below that range.
I wrote a small script to test this:
var hd = require('./humanize-duration');
const {y, mo, w, d, h, m, s, ms} = hd.unitMeasures
const col = columnNumber => `\x1b[${columnNumber}G`
function printColumns(...columns) {
console.log(`%s${col(15)}%s${col(35)}%s`, ...columns)
}
function testTime(t) {
printColumns(t, hd(t, { largest: 2, round: Math.ceil }), hd(t))
}
printColumns('Time', 'Round: Math.ceil', 'Round: false, largest: undefined')
console.log();
testTime(1*y + 11*mo)
testTime(1*y + 11*mo + 1*ms)
console.log();
testTime(1*y + 11*mo + 2*w + 3*d)
testTime(1*y + 11*mo + 2*w + 4*d)
console.log();
testTime(1*y + 11*mo + 4*w)
testTime(1*y + 11*mo + 4*w + 1*ms)
console.log();
testTime(2*y - 1*ms)
testTime(2*y)
testTime(2*y + 1*ms)
Running:
node compare-rounding.js
Time Round: Math.ceil Round: false, largest: undefined
60485400000 1 year, 11 months 1 year, 11 months
60485400001 2 years 1 year, 11 months, 0.001 seconds
61954200000 2 years 1 year, 11 months, 2 weeks, 3 days
62040600000 2 years 1 year, 11 months, 2 weeks, 4 days
62904600000 2 years 1 year, 11 months, 4 weeks
62904600001 1 year, 13 months 1 year, 11 months, 4 weeks, 0.001 seconds
63115199999 1 year, 13 months 1 year, 11 months, 4 weeks, 2 days, 10 hours, 29 minutes, 59.999 seconds
63115200000 2 years 2 years
63115200001 2 years, 1 month 2 years, 0.001 seconds
OK, fixed.
The fix does involve changing some existing behavior (specifically when using largest
without enabling round
) so if this gets merged, the next release should probably be a major version bump.
Unlike before, rounding is now only performed if the round
option is set.
Previously:
n_ > var t = 540360012
undefined
n_ > hd(t)
'6 days, 6 hours, 6 minutes, 0.012 seconds'
n_ > hd(t, { largest: 2 })
'6 days, 6 hours'
n_ > hd(t, { round: true })
'6 days, 6 hours, 6 minutes'
n_ > hd(t, { largest: 2, round: true })
'6 days, 6 hours'
n_ > hd(t, { units: ['d', 'h', 'm'] })
'6 days, 6 hours, 6.0002 minutes'
n_ > hd(t, { units: ['d', 'h', 'm'], largest: 2 })
'6 days, 6 hours'
n_ > hd(t, { units: ['d', 'h'], largest: 2 })
'6 days, 6.100003333333333 hours'
n_ > hd(t, { units: ['d', 'h', 'm'], round: true })
'6 days, 6 hours, 6 minutes'
n_ > hd(t, { units: ['d', 'h', 'm'], largest: 2, round: true })
'6 days, 6 hours'
n_ > hd(t, { units: ['d', 'h'], largest: 2, round: true })
'6 days, 6 hours'
Now:
n_ > var t = 540360012
undefined
n_ > hd(t)
'6 days, 6 hours, 6 minutes, 0.012 seconds'
n_ > hd(t, { largest: 2 })
'6 days, 6.100003333333333 hours'
n_ > hd(t, { round: true })
'6 days, 6 hours, 6 minutes'
n_ > hd(t, { largest: 2, round: true })
'6 days, 6 hours'
n_ > hd(t, { units: ['d', 'h', 'm'] })
'6 days, 6 hours, 6.0002 minutes'
n_ > hd(t, { units: ['d', 'h', 'm'], largest: 2 })
'6 days, 6.100003333333333 hours'
n_ > hd(t, { units: ['d', 'h'], largest: 2 })
'6 days, 6.100003333333333 hours'
n_ > hd(t, { units: ['d', 'h', 'm'], round: true })
'6 days, 6 hours, 6 minutes'
n_ > hd(t, { units: ['d', 'h', 'm'], largest: 2, round: true })
'6 days, 6 hours'
n_ > hd(t, { units: ['d', 'h'], largest: 2, round: true })
'6 days, 6 hours'
@whitelynx Any updates on this?
There hasn't been movement on this in months and this module is in maintenance mode and won't be accepting new features (see #120), so I'm going to close this PR.
Two test cases in the unit test for usage with the
largest
option currently fail in ways that suggest issues in other code.Some background on my motivations for this change: Since version 2.0.0, Moment.js has a different default rounding scheme than HumanizeDuration.js. (though that is configurable) In many cases Moment.js's default of always rounding toward zero (effectively using
Math.floor()
for positive numbers) makes sense; for instance, if you're rounding to weeks, it may not be desirable to show "2 weeks" until you've actually hit 14 days, and instead display e.g. "1 week" if the duration is 13 days.