Duration#toHuman does not print human-readable strings
#1134
Comments
|
Additional to your modified Duration.fromObject({seconds: 180}).toHuman() => "3 minutes and 0 seconds"
Duration.fromObject({seconds: 181}).toHuman() => "3 minutes and 1 second"
Duration.fromObject({seconds: 3600}).toHuman() => "1 hour, 0 minutes and 0 seconds"
Duration.fromObject({seconds: 3601}).toHuman() => "1 hour, 0 minutes and 1 seconds"
Duration.fromObject({seconds: 3800}).toHuman() => "1 hour, 3 minutes and 20 seconds"
Duration.fromObject({seconds: 3800}).toHuman({ unitDisplay: "short" }) => "1 hr, 3 mins, 20 secs"And I would like to see some new options for the Config Option:
|
|
I could really use |
Changing the units would change the duration in destructive ways (e.g. days are not all the same length). If you want to change the units represented, you have to change them in the duration (e.g. with
Agreed, we should strip the zeros for non-smallest unit
Agreed I'd take a PR that reflects that. |
|
Hi there, I'd just like to bump this issue. I would love to see some of the changes proposed by @orphic-lacuna and @paldepind, especially those focused on stripping 0 values and dealing with them within the
It looks like this is a relatively old PR/issue. Are there any updates or other info on this being implemented soon? |
|
Hi super keen on this as well, wanted to use Luxon as I'd heard good things. Docs still say "Luxon does not (yet) have an equivalent of Moment's Duration humanize method. Luxon will add that when Intl.UnitFormat is supported by browsers." Yeah an ideal solution might also allow setting the singular/plural unit names for i18n. Potentially interesting with polyfills? |
|
Yes, this is preventing us from using Luxon, will keep moment for now. |
|
This is a simple work-around that might help others? As a case-by-case basis you can pluck off what you need. I know this could be simplified, but a simple possible "fix"? const testDuration = Duration.fromObject({ seconds: 290});
let testString = '';
if (testDuration .valueOf()) {
const { hours, minutes, seconds } = Duration.fromISOTime(testDuration .toISOTime());
if (hours) {
if (hours > 1) {
testString += `${hours} hours `;
else {
testString += `${hours} hour `;
}
}
if (minutes) {
if (minutes > 1) {
testString += `${minutes} minutes `;
} else {
testString += `${minutes} minute `;
}
}
if (seconds) {
if (seconds > 1) {
testString += `${seconds} seconds`;
} else {
testString += `${seconds} second`;
}
}
} |
@orphic-lacuna Thank you very much for sharing your version! I converted it to typescript (just very few changes were required), however, I didn't figure out yet (I just recently started using typescript) how to extend the (Duration.prototype as any).__toHuman__ = Duration.prototype.toHuman;
(Duration.prototype as any).toHuman = function (
opts: ToHumanDurationOptions & {
stripZeroUnits?: 'all' | 'end' | 'none';
precision?: DurationLikeObject;
maxUnits?: number;
smallestUnit?: DurationUnit;
biggestUnit?: DurationUnit;
} = { stripZeroUnits: 'all' },
): string {
let duration = this.normalize();
let durationUnits = [];
let precision =
typeof opts.precision == 'object'
? Duration.fromObject(opts.precision)
: Duration.fromMillis(0);
let remainingDuration = duration;
//list of all available units
const allUnits = [
'years',
'months',
'days',
'hours',
'minutes',
'seconds',
'milliseconds',
];
let smallestUnitIndex;
let biggestUnitIndex;
// check if user has specified the smallest unit that should be displayed
if (opts.smallestUnit) {
smallestUnitIndex = allUnits.indexOf(opts.smallestUnit);
}
// check if user has specified a biggest unit
if (opts.biggestUnit) {
biggestUnitIndex = allUnits.indexOf(opts.biggestUnit);
}
// use seconds and years as default for smallest and biggest unit
if (
!smallestUnitIndex ||
!(smallestUnitIndex >= 0 && smallestUnitIndex < allUnits.length)
)
smallestUnitIndex = allUnits.indexOf('seconds');
if (
!biggestUnitIndex ||
!(
biggestUnitIndex <= smallestUnitIndex &&
biggestUnitIndex < allUnits.length
)
)
biggestUnitIndex = allUnits.indexOf('years');
for (let unit of allUnits.slice(biggestUnitIndex, smallestUnitIndex + 1)) {
const durationInUnit = remainingDuration.as(unit);
if (durationInUnit >= 1) {
durationUnits.push(unit);
let tmp: any = {};
tmp[unit] = Math.floor(remainingDuration.as(unit));
remainingDuration = remainingDuration
.minus(Duration.fromObject(tmp))
.normalize();
// check if remaining duration is smaller than precision
if (remainingDuration < precision) {
// ok, we're allowed to remove the remaining parts and to round the current unit
break;
}
}
// check if we have already the maximum count of units allowed
if (opts.maxUnits && durationUnits.length >= opts.maxUnits) {
break;
}
}
// after gathering of units that shall be displayed has finished, remove the remaining duration to avoid non-integers
duration = duration.minus(remainingDuration).normalize();
duration = duration.shiftTo(...durationUnits);
if (opts.stripZeroUnits == 'all') {
durationUnits = durationUnits.filter((unit) => duration.values[unit] > 0);
} else if (opts.stripZeroUnits == 'end') {
let mayStrip = true;
durationUnits = durationUnits.reverse().filter((unit /*index*/) => {
if (!mayStrip) return true;
if (duration.values[unit] == 0) {
return false;
} else {
mayStrip = false;
}
return true;
});
}
return duration.shiftTo(...durationUnits).__toHuman__(opts);
}; |
Is someone working on a PR? As we currently need this, I could help. |
|
I'm currently not working on a PR. Feel free to create it if you like :) |
Me neither. Would be great if your could help 👍 |
|
Alright, hope I can invest some time this week, not sure yet. |
|
Minor update in the meantime: In case of zero length Durations, I prefer to output only a single zero unit (not "0 years, 0 month, 0 days, ..."). We could make it also configurable which one, but for now I rather use the smallest unit in this case. Added the if right before the last line. (Duration.prototype as any).__toHuman__ = Duration.prototype.toHuman;
(Duration.prototype as any).toHuman = function (
opts: ToHumanDurationOptions & {
stripZeroUnits?: 'all' | 'end' | 'none';
precision?: DurationLikeObject;
maxUnits?: number;
smallestUnit?: DurationUnit;
biggestUnit?: DurationUnit;
} = { stripZeroUnits: 'all' },
): string {
let duration = this.normalize();
let durationUnits = [];
let precision =
typeof opts.precision == 'object'
? Duration.fromObject(opts.precision)
: Duration.fromMillis(0);
let remainingDuration = duration;
//list of all available units
const allUnits = [
'years',
'months',
'days',
'hours',
'minutes',
'seconds',
'milliseconds',
];
let smallestUnitIndex;
let biggestUnitIndex;
// check if user has specified the smallest unit that should be displayed
if (opts.smallestUnit) {
smallestUnitIndex = allUnits.indexOf(opts.smallestUnit);
}
// check if user has specified a biggest unit
if (opts.biggestUnit) {
biggestUnitIndex = allUnits.indexOf(opts.biggestUnit);
}
// use seconds and years as default for smallest and biggest unit
if (
!smallestUnitIndex ||
!(smallestUnitIndex >= 0 && smallestUnitIndex < allUnits.length)
)
smallestUnitIndex = allUnits.indexOf('seconds');
if (
!biggestUnitIndex ||
!(
biggestUnitIndex <= smallestUnitIndex &&
biggestUnitIndex < allUnits.length
)
)
biggestUnitIndex = allUnits.indexOf('years');
for (let unit of allUnits.slice(biggestUnitIndex, smallestUnitIndex + 1)) {
const durationInUnit = remainingDuration.as(unit);
if (durationInUnit >= 1) {
durationUnits.push(unit);
let tmp: any = {};
tmp[unit] = Math.floor(remainingDuration.as(unit));
remainingDuration = remainingDuration
.minus(Duration.fromObject(tmp))
.normalize();
// check if remaining duration is smaller than precision
if (remainingDuration < precision) {
// ok, we're allowed to remove the remaining parts and to round the current unit
break;
}
}
// check if we have already the maximum count of units allowed
if (opts.maxUnits && durationUnits.length >= opts.maxUnits) {
break;
}
}
// after gathering of units that shall be displayed has finished, remove the remaining duration to avoid non-integers
duration = duration.minus(remainingDuration).normalize();
duration = duration.shiftTo(...durationUnits);
if (opts.stripZeroUnits == 'all') {
durationUnits = durationUnits.filter((unit) => duration.values[unit] > 0);
} else if (opts.stripZeroUnits == 'end') {
let mayStrip = true;
durationUnits = durationUnits.reverse().filter((unit /*, index*/) => {
if (!mayStrip) return true;
if (duration.values[unit] == 0) {
return false;
} else {
mayStrip = false;
}
return true;
});
}
// if `durationUnits` is empty (i.e. duration is zero), then just shift to the smallest unit
if (!durationUnits.length) {
durationUnits.push(allUnits[smallestUnitIndex]);
}
return duration.shiftTo(...durationUnits).__toHuman__(opts);
}; |
|
I'd like to extract the functionality that handles the added options from your |
|
I also have a fix that the precision works as expected that the value is rounded to the precision and I can have |
|
I'm not sure, if I understood the point exactly ... You want to separate this functionality, because modifying the units of the duration would alter the duration object in a destructive way as mentioned by @icambron , right? For me it seems reasonable to separate it, for example like this: // create duration object
let d = Duration.fromObject({seconds: 3601});
// convert internal representation in different units to a human readable format, hours would be 1, and seconds 1
let humanizedDuration = d.normalize({... config options go in here});
// toHuman() is just a "stringifyer method": it would print something like "1 hour and 1 seond" based on the config options that were passed to .normalize()
console.log(humanizedDuration.toHuman());The actual magic would happen in |
Well, nearly. In any case the normalize/humanize operation should not alter the duration object. The reason is as you mentioned, that
|
|
I just found the new release and !1299 that adds the However, I feel that it makes sense to think a wee bit about it and ensure that we get an overall consistent design. Some questions I think of:
What do you think? |
|
Proposal:
WRT implementation:
Looking forward to hear you comments! |
|
Yeah, having function removeZeroes(vals) {
const newVals = {};
for (const [key, value] of Object.entries(vals)) {
if (value !== 0) {
newVals[key] = value;
}
}
return newVals;
}
const vals = removeZeroes(Duration.fromObject({ seconds: estimatedSeconds }).normalize().shiftTo("hours", "minutes", "seconds").toObject());
const timeLeft = Duration.fromObject(vals).toHuman({ unitDisplay: "narrow", maximumFractionDigits: 0 });I couldn't just use |
|
I just have a case where I'd really like to have |
|
I think |
|
Agreed. To me the ideal interface would be Duration.fromObject({ seconds: 12303 })
.toHuman({ smallestUnit: "seconds", largestUnit: "hours", unitDisplay: "narrow" })
=> "3h, 25m and 3s"Which is what the code I commented before does. Zeroes always removed, maximumFractionDigits 0 by default but can be overridden. |
|
I also agree. The question is only, do we want to introduce a breaking change? To keep the previous behavior of AND: I really would like to be able to find-tune the Duration even when I don't want to renter it to text. And then we're at my proposal from above, with the optional opts argument also supported by Before starting to adjust the MR, I'd like to hear some feedback from the maintainers to get a go. |
|
Definitely introduce the breaking change. It is a straight up bug that doesn't do what it is supposed to do, making it next to unusable. Forcing someone to add Since it is broken, so now is the time to fix it. 99 out of a 100 usages all have to wrap this functionality to normalize and remove zeros in order to do what they want. If we make this change it only forces it to do what, by default, it promises to do. |
|
I haven't had a chance to catch up on this (long) issue and the draft PR associated with it. Just dropping in a note to say I see it and am not ignoring it |
Thanks @icambron for your ping 🙂 |
|
Ok, finally getting to this. Sorry it has taken so long. Comments:
I dislike having lots of options on things. Easier to have separate methods where it makes sense. The more options you add, the more places where you have say "option x only works if option y is not provided" or other weird stuff.
Overall, one of my reactions here is that everyone seems to expect I am open to adding useful additional options to the more narrowly-tailored methods like
Good catch. There shouldn't be a I hope that helps! |
|
Isn't #1299 resolving this issue? at least partially? For sure @orphic-lacuna suggested some more features ( |
|
Just wanted to add that it would be nice if for |
|
This is my attempt to improve it: export function toHumanDuration(start: DateTime, end: DateTime): string {
// Better Duration.toHuman support https://github.com/moment/luxon/issues/1134
const duration = end
.diff(start)
.shiftTo("days", "hours", "minutes", "seconds")
.toObject();
if ("seconds" in duration) {
duration.seconds = Math.round(duration.seconds!);
}
const cleanedDuration = Object.fromEntries(
Object.entries(duration).filter(([_k, v]) => v !== 0)
) as DurationObjectUnits;
if (Object.keys(cleanedDuration).length === 0) {
cleanedDuration.seconds = 0;
}
return Duration.fromObject(cleanedDuration).toHuman();
} |
|
The |
|
Here's my implementation, drawing from @seyeong. This adds suffix and prefix, and makes the duration values an absolute value, which mimics export const toAbsHumanDuration = (start: DateTime, end: DateTime): string => {
// Better Duration.toHuman support https://github.com/moment/luxon/issues/1134
const duration = end.diff(start).shiftTo('days', 'hours', 'minutes', 'seconds').toObject();
const prefix = start > end ? 'in ' : '';
const suffix = end > start ? ' ago' : '';
if ('seconds' in duration) {
duration.seconds = Math.round(duration.seconds!);
}
const cleanedDuration = Object.fromEntries(
Object.entries(duration)
.filter(([_key, value]) => value !== 0)
.map(([key, value]) => [key, Math.abs(value)])
) as DurationObjectUnits;
if (Object.keys(cleanedDuration).length === 0) {
cleanedDuration.seconds = 0;
}
const human = Duration.fromObject(cleanedDuration).toHuman();
return `${prefix}${human}${suffix}`;
}; |
|
My two cents: Uses I wish there was a |
You can use: |
|
Want to give a hearty +1 to making After initially looking through the comments, I had thought of adding something like Lines 673 to 687 in cea7b5f i.e. Something like this: /**
* Filter this Duration by the supplied filter function. Return a newly-constructed Duration.
* @param {function} fn - The function to apply to each unit. Arity is 1 or 2: the value of the unit and, optionally, the unit name. Must return a boolean.
* @example Duration.fromObject({ days: 0, hours: 1, minutes: 15 }).filterUnits(x => x !== 0) //=> { hours: 1, minutes: 15 }
* @example Duration.fromObject({ days: 0, hours: 1, minutes: 15 }).mapUnits((x, u) => u === "minutes" && x < 30) //=> { days: 0, hours: 1 }
* @return {Duration}
*/
filterUnits(fn) {
if (!this.isValid) return this;
const result = {};
for (const k of Object.keys(this.values)) {
if (fn(this.values[k], k)) result[k] = this.values[k];
}
return clone(this, { values: result }, true);
}It would be amazing to have a /**
* @example Duration.fromObject({ years: 1, months: 5, days: 0, hours: 1, minutes: 15 }).sliceUnits(0, 2) //=> { years: 1, months: 5 }
* @example Duration.fromObject({ days: 0, hours: 1, minutes: 15 }).sliceUnits(0, 2) //=> { days: 0, hours: 1 }
* @example Duration.fromObject({ days: 0, hours: 1, minutes: 15 }).filterUnits(x => x !== 0).sliceUnits(0, 2) //=> { hours: 1, minutes: 15 }
*/If there is willingness for one or both of these approaches, let me know; I don't mind putting a PR together with some tests in it! In the meantime, if this isn't actioned and future readers/future me are looking to solve this, I got the same result with this large-and-ugly but not-a-monkey-patch snippet: Duration.fromObject(
Object.fromEntries(
Object.entries(
DateTime.fromSeconds( newTimestamp )
.diff( DateTime.fromSeconds( oldTimestamp ) )
.rescale()
.toObject()
).filter( ([ , value ]) => value !== 0 )
.slice( 0, 2 )
)
).toHuman({ listStyle: 'long', maximumFractionDigits: 0 }) |
Based on the name I would expect that
Duration#toHumanreturns strings that are optimal for human reading. But, IMHO this is not the case.The way I see it there are three issues.
Durationis created from a number of milliseconds then the milliseconds are printed as-is, which is not human-readable.In this case, I would expect something like the string "6 hours and 9 minutes" which is what humanize-duration returns.
Durationis created with some units being zero then those units are printed even though they do not benefit humans.Durationis created with an empty object then an empty string is returned. An empty string is not a human-readable duration.It seems to me that
toHumanonly returns the internal representation of the duration with unit names attached and not actual human-friendly output.I think that either the
toHumanmethod should be renamed as the current name is confusing or the above problems should be fixed. Units that are zero should not be printed and small units should be converted into larger units if possible. To fix the last issue I think thattoHumanshould accept a (potentially optional) second argument that specifies the smallest unit to print. This argument can be used to not print unwanted precision (for instance milliseconds) and will be used as the fallback unit to print if the duration contains no units.Here is a small proof of concept of what I suggest:
For the above examples this implementation behaves as follows:
The text was updated successfully, but these errors were encountered: