luxon: `Duration#toHuman` does not print human-readable strings
Based on the name I would expect that Duration#toHuman
returns strings that are optimal for human reading. But, IMHO this is not the case.
The way I see it there are three issues.
- If a
Duration
is created from a number of milliseconds then the milliseconds are printed as-is, which is not human-readable.
Duration.fromMillis(22140000).toHuman() //=> "22140000 milliseconds"
In this case, I would expect something like the string “6 hours and 9 minutes” which is what humanize-duration returns.
- If a
Duration
is created with some units being zero then those units are printed even though they do not benefit humans.
Duration.fromMillis(3 * 60 * 1000).shiftTo("hours", "minutes").toHuman()
//=> "0 hours, 3 minutes"
Duration.fromObject({ days: 0, hours: 0, minutes: 12, seconds: 0 }).toHuman()
//=> "0 days, 0 hours, 12 minutes, 0 seconds"
- If a
Duration
is created with an empty object then an empty string is returned. An empty string is not a human-readable duration.
Duration.fromObject({}).toHuman() //=> ""
It seems to me that toHuman
only returns the internal representation of the duration with unit names attached and not actual human-friendly output.
I think that either the toHuman
method 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 that toHuman
should 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:
function toHuman(dur: Duration, smallestUnit = "seconds"): string {
const units = ["years", "months", "days", "hours", "minutes", "seconds", "milliseconds", ];
const smallestIdx = units.indexOf(smallestUnit);
const entries = Object.entries(
dur.shiftTo(...units).normalize().toObject()
).filter(([_unit, amount], idx) => amount > 0 && idx <= smallestIdx);
const dur2 = Duration.fromObject(
entries.length === 0 ? { [smallestUnit]: 0 } : Object.fromEntries(entries)
);
return dur2.toHuman();
}
For the above examples this implementation behaves as follows:
toHuman(Duration.fromMillis(3 * 60 * 1000)) //=> "3 minutes"
toHuman(Duration.fromObject({ days: 0, hours: 0, minutes: 12, seconds: 0 })) //=> "12 minutes"
toHuman(Duration.fromObject({})) //=> "0 seconds"
About this issue
- Original URL
- State: open
- Created 2 years ago
- Reactions: 102
- Comments: 35 (17 by maintainers)
Commits related to this issue
- #1134 extended Duration.normalize(), first version — committed to etalytics/luxon by deleted user 2 years ago
Additional to your modified
toHuman()
method I’d like to go even further. In my opinion the.toHuman()
method should have the following behaviour, some examples:And I would like to see some new options for the
.toHuman()
method (with examples):Config Option:
stripZeroUnits
Removes all zero parts from the human-readable string. Allowed values are
all
which would remove all parts that are zero, orend
which would only remove zero parts at the end of the text. Examples:Config Option:
precision
Determines a minimum precision of the human-readable string. All parts of it which are smaller than the specified precision will be omitted. Examples:
Config Option:
maxUnits
An integer that determines the maximum allowed number of parts. Examples:
Config Options:
smallestUnit
andbiggestUnit
Determine the biggest and smallest unit that should be used. Default values are
smallestUnit: "seconds"
andbiggestUnit: "years"
. Examples:To achieve this behaviour I have written the following wrapper function for the
.toHuman()
method:According to the contribution guidelines one should ask first if Luxon wants to integrate this or that feature before putting too much effort into this … So here it is:
Do you think this feature set should be included into Luxon? If so, I could make a pull request if you want.
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
toHuman()
function. Specifically elegantly resolving the following:Duration.fromObject({ days: 0, hours: 0, minutes: 75 }).toHuman() // => 0 days, 1 hours, 15 minutes, 0 seconds
It looks like this is a relatively old PR/issue. Are there any updates or other info on this being implemented soon?
Yes, this is preventing us from using Luxon, will keep moment for now.
I think
toHuman
has to normalize and remove zeros by default. It’s calledtoHuman
. The proposal makes it sound like we want the toHuman to really be toFormat(LONG). I don’t think we can call it to human to show zero duration elements or 3600 seconds. No one says that.Proposal:
normalize()
without arguments should keep the behavior as it isnormalize(opts)
is extended as described aboverescale()
is then just a shorthand fornormalize({ stripZeroUnits: 'all', precision: { milliseconds: 1}, smallestUnit: "milliseconds" })
that should be kept as it is now part of the Duration interface.toHuman()
which could then callnormalize(opts)
, because this is a behavior that one could expect, but I prefer the idea to have clean separation of functionality and usenormalize
to adjust andtoHuman
just to format.removeZeros(atStartAndEndOnly = false)
to Duration’s interface, and when the argument istrue
, only zero units from start and end are removed and intermediates are kept, as in the example above, e.g. to get1 hour, 0 minutes, and 5 seconds
. In fact, I suggest to ‘trim’ zeros in this case and not remove just from end as above. Or do you think there are cases when it’s required to keep zeros at the start or at the end?normalize(ops.stripZeroUnits)
should be renamed tonormalize(ops.removeZeroUnits)
to be consistentremoveZeroUnits
would then be'all'
and'trim'
(and in case we decide that we need it also'start'
and'end'
)Duration.round(precision)
to the interface.normalize(opts)
is then a combined call toround(opts.precision)
,rescale(opts.smallestUnit, biggestUnit, maxUnits)
, andremoveZeros(opts.removeZeroUnits === 'trim')
. I actually like this.normalize()
then when we introduce the other 3 methods. However, if feel that the currentnormalize()
's functionality is not what one (well, me at least 😛) would intuitively expect, so the options might help to adjust user’s expectations.WRT implementation:
opts.maxUnits
is defined, I guess it’s more efficient to useshiftTo()
as in @NGPixel’s implementation ofrescale()
with units adjusted byopts.smallestUnit
andopts.biggestUnit
, but I guess for maxUnits @orphic-lacuna’s implmentation from above can be used. Or is there a more efficient way to archive this?normalize()
is actually needed. When I useshiftTo()
does it normalize? In the current implementation ofrescale()
this is done:removeZeroes(this.normalize().shiftToAll().toObject())
- is thenormalize()
required here? (I didn’t try it without yet.)Looking forward to hear you comments!
@icambron @orphic-lacuna @paldepind @NGPixel
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
opts
just so it does the right thing by default is going to make it a pain for everyone without any benefit.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.
Agreed. To me the ideal interface would be
Which is what the code I commented before does.
Zeroes always removed, maximumFractionDigits 0 by default but can be overridden.
Here’s my implementation, drawing from @seyeong. This adds suffix and prefix, and makes the duration values an absolute value, which mimics
date-fns
formatDistance
output:Just wanted to add that it would be nice if for
maxUnits
we could also specify another option likeroundingMethod
with values'floor' | 'ceil' | 'round'
. date-fns has this feature and if this was added I wouldn’t use date-fns for just formatting distances between dates@ptandler
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.
removeZeros
works directly on the values, so it should be private. We should add a public method with the same name that wraps that with aclone
call. That just wasn’t implemented.Overall, one of my reactions here is that everyone seems to expect
normalize
to do a big complicated thing when it is trying to do a simple thing (that happens to have a complicated implementation). Maybenormalize
was not a good name for what it does, but it seems better to add the functionality we want separately than to try to jam a different sense of what “normalize” should mean into an existing method with limited scope. Normalize’s job is to move values between the existing units. Anything that messes with the set of units needs to be really explicit. That’s why I want separate methods instead of a zillion options fornormalize
. It would be a monster with a mission like “do stuff to durations”I am open to adding useful additional options to the more narrowly-tailored methods like
rescale
, and passing them through to a new public version ofremoveZeroes
.trimOnly
is what I’d call that optionGood catch. There shouldn’t be a
normalize
there; theshiftTo
call insideshiftToAll
does the normalization, so there shouldn’t need to be one directly inrescale
I hope that helps!
Well, nearly. In any case the normalize/humanize operation should not alter the duration object. The reason is as you mentioned, that
toHuman()
is very clear as just being a stringifier method and you could use the improvednormalize()
in other contexts as well if it’s separated.Hi super keen on this as well, wanted to use Luxon as I’d heard good things. But seems I’ll have to keep using moment for now.
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.” Pull request below was merged to update this text I guess it is not uploaded to docs?
Yeah an ideal solution might also allow setting the singular/plural unit names for i18n. Related: https://github.com/moment/luxon/pull/785
Potentially interesting with polyfills? https://formatjs.io/docs/polyfills/intl-numberformat
You can use: .toHuman({maximumFractionDigits: 0})
My two cents:
Uses
negate()
to make past dates take positive duration meaning, thenMath.floor
to round off to the nearest day, and finallyrescale
to express it in terms of larger units.I wish there was a
toHuman({round: "days"})
which would act the same as what I just described.Isn’t https://github.com/moment/luxon/pull/1299 resolving this issue? at least partially?
For sure @orphic-lacuna suggested some more features (
precision
,maxUnits
,smallestUnit
,biggestUnit
) in his/her first comment here… Should these suggested formatting features be moved to another enhancement issue ?I just have a case where I’d really like to have
removeZeroes()
in the Duration interface.Yeah, having
removeZeroes
be private made have to copy it into my code…I couldn’t just use
rescale()
because I don’t want the milliseconds (shiftTo("hours", "minutes", "seconds")
).I just found the new release and !1299 that adds the
rescale()
andshiftToAll()
functions which already provides a good part of the functionality we discussed here. The version from above that I started turning into !1323 adds more options, though.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:
rescale()
or to add options tonormalize()
?removeZeros()
implemented as private function not as part of the interface?normalize()
in addition to the newrescale()
?rescale()
? IMO the originalnormalize()
is not intuitive and provides not what I did expect.What do you think?
@icambron @orphic-lacuna @paldepind