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

Most upvoted comments

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:

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 .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, or end which would only remove zero parts at the end of the text. Examples:

Duration.fromObject({seconds: 180}).toHuman({stripZeroUnits: "end"}) => "3 minutes"
Duration.fromObject({seconds: 3660}).toHuman({stripZeroUnits: "end"}) => "1 hour and 1 minute"
Duration.fromObject({seconds: 3601}).toHuman({stripZeroUnits: "all"}) => "1 hour and 1 second"

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:

Duration.fromObject({seconds: 3800}).toHuman({precision: {minutes: 5}}) => "1 hour"
Duration.fromObject({seconds: 3800}).toHuman({precision: {minutes: 2}}) => "1 hour and 3 minutes"

Config Option: maxUnits

An integer that determines the maximum allowed number of parts. Examples:

Duration.fromObject({seconds: 3661}).toHuman({maxUnits: 2}) => "1 hour and 1 minute"

Config Options: smallestUnit and biggestUnit

Determine the biggest and smallest unit that should be used. Default values are smallestUnit: "seconds" and biggestUnit: "years". Examples:

Duration.fromObject({seconds: 3661}).toHuman({biggestUnit: "minutes"}) => "61 minutes and 1 second"
Duration.fromObject({seconds: 3661}).toHuman({smallestUnit: "hours"}) => "1 hour"

To achieve this behaviour I have written the following wrapper function for the .toHuman() method:

const Duration = luxon.Duration;
Duration.prototype.__toHuman__ = Duration.prototype.toHuman;
Duration.prototype.toHuman = function(opts = {}) {
	let duration = this.normalize();
	let durationUnits = [];
	let precision;
	if (typeof opts.precision == "object") {
		precision = Duration.fromObject(opts.precision);
	}
	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 a 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 >= 0) && (smallestUnitIndex < allUnits.length))) smallestUnitIndex = allUnits.indexOf("seconds");
	if (!((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 = {};
			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 (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);
}

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 called toHuman. 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 is
  • normalize(opts) is extended as described above
  • rescale() is then just a shorthand for normalize({ stripZeroUnits: 'all', precision: { milliseconds: 1}, smallestUnit: "milliseconds" }) that should be kept as it is now part of the Duration interface.
  • I’m tempted to allow to pass the options also th toHuman() which could then call normalize(opts), because this is a behavior that one could expect, but I prefer the idea to have clean separation of functionality and use normalize to adjust and toHuman just to format.
  • Add removeZeros(atStartAndEndOnly = false) to Duration’s interface, and when the argument is true, only zero units from start and end are removed and intermediates are kept, as in the example above, e.g. to get 1 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 to normalize(ops.removeZeroUnits) to be consistent
  • the options of removeZeroUnits would then be 'all' and 'trim' (and in case we decide that we need it also 'start' and 'end')
  • We could think of also extracting Duration.round(precision) to the interface.
  • normalize(opts) is then a combined call to round(opts.precision), rescale(opts.smallestUnit, biggestUnit, maxUnits), and removeZeros(opts.removeZeroUnits === 'trim'). I actually like this.
  • In terms of redundancy one could argue that we don’t need to extend normalize() then when we introduce the other 3 methods. However, if feel that the current normalize()'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:

  • when no opts.maxUnits is defined, I guess it’s more efficient to use shiftTo() as in @NGPixel’s implementation of rescale()with units adjusted by opts.smallestUnit and opts.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?
  • I don’t understand in which cases the current normalize() is actually needed. When I use shiftTo() does it normalize? In the current implementation of rescale() this is done: removeZeroes(this.normalize().shiftToAll().toObject()) - is the normalize() 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

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.

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:

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}`;
};

Just wanted to add that it would be nice if for maxUnits we could also specify another option like roundingMethod 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:

Better to add new functions such as rescale() or to add options to normalize()?

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.

Why was removeZeros() implemented as private function not as part of the interface?

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 a clone call. That just wasn’t implemented.

Or would the additional options better fit to rescale()? IMO the original normalize() is not intuitive and provides not what I did expect.

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). Maybe normalize 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 for normalize. 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 of removeZeroes.

Add removeZeros(atStartAndEndOnly = false) to Duration’s interface, and when the argument is true, only zero units from start and end are removed and intermediates are kept, as in the https://github.com/moment/luxon/issues/1134#issuecomment-1033896418, e.g. to get 1 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?

trimOnly is what I’d call that option

I don’t understand in which cases the current normalize() is actually needed. When I use shiftTo() does it normalize? In the current implementation of rescale() this is done: removeZeroes(this.normalize().shiftToAll().toObject()) - is the normalize() required here? (I didn’t try it without yet.)

Good catch. There shouldn’t be a normalize there; the shiftTo call inside shiftToAll does the normalization, so there shouldn’t need to be one directly in rescale

I hope that helps!

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?

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 improved normalize() in other contexts as well if it’s separated.

// 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());

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

My two cents:

const durationAgo = (timestamp: DateTime, round: DurationUnit = "days") => timestamp.diffNow([round]).negate().mapUnits(x => Math.floor(x)).rescale().toHuman();

Uses negate() to make past dates take positive duration meaning, then Math.floor to round off to the nearest day, and finally rescale 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.

You can use: .toHuman({maximumFractionDigits: 0})

My two cents:

const durationAgo = (timestamp: DateTime, round: DurationUnit = "days") => timestamp.diffNow([round]).negate().mapUnits(x => Math.floor(x)).rescale().toHuman();

Uses negate() to make past dates take positive duration meaning, then Math.floor to round off to the nearest day, and finally rescale 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…

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 rescale() because I don’t want the milliseconds (shiftTo("hours", "minutes", "seconds")).

I just found the new release and !1299 that adds the rescale() and shiftToAll() 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:

  • Better to add new functions such as rescale() or to add options to normalize()?
  • Which of the options from above are really usefull? Actually, I quite like to be able to define smallest and biggest and max units, and also precision.
  • Why was removeZeros() implemented as private function not as part of the interface?
  • Would it harm (in terms of usability, flexibility, clearness, code size, …) to also add options to normalize() in addition to the new rescale()?
  • Or would the additional options better fit to rescale()? IMO the original normalize() is not intuitive and provides not what I did expect.

What do you think?

@icambron @orphic-lacuna @paldepind