Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More control over DateTime#toRelative rounding behavior #1129

Open
corymharper opened this issue Jan 28, 2022 · 10 comments · May be fixed by #1685
Open

More control over DateTime#toRelative rounding behavior #1129

corymharper opened this issue Jan 28, 2022 · 10 comments · May be fixed by #1685

Comments

@corymharper
Copy link

Is your feature request related to a problem? Please describe.
For some units of time, rounding down is very unintuitive. It's probably fine if 29.99 minutes rounds down to 29, but it's fairly inaccurate to describe 1.99 years away as "1 year away." Padding in these situations that could be minutes away or years away can lead to a lot of extra code to figure out how much padding there should be in this situation.

Describe the solution you'd like
I think one possible solution would be to change the round option you can pass to toRelative from a on/off style boolean into a setting probably based on a string. Something like round: 'down' | 'up' | 'closest' | 'off' where, if we want to keep the current default, it defaults to down. off would be the equivalent of false currently, up would invert the behavior and always round upwards, and closest would round towards the closest integer, rather than specifically up or down.

Describe alternatives you've considered
With the existing options the only alternatives I've come up with are calculating the padding based on the kind of rounding I want based on the difference between the date for comparison and now, or altering the date for comparison itself to generate the term I want when handed to toRelative. An alternative to the addition I suggest above might be to allow handing a function to the round option that would receive a parameter of the unrounded relative value, and the resulting number would be whatever is returned from that function.

@NfNitLoop
Copy link

This "rounding down" behavior is especially strange when using toRelative() to show a countdown timer.

With the default behavior, you'll see "1 minute" for a full 60 seconds before you see "59 seconds" and realize now you've actually got a minute left.

I was hopeful when I saw #926 because I thought specifying "multiple units" like ["minutes", "seconds"] would allow me to see output like in 1 minute 23 seconds, but that's not how that behaves. (It continues to pick a single unit, from among those choices.)

So maybe an option like significantUnits would help here. (Please feel free to tell me to open a separate issue for this if you'd like to consider it separately. I'm mostly adding it to this ticket for some example use cases.) To take the above 29.99 minutes as an example, I'd expect .toRelative({significantUnits: 2}) to output in 29 minutes 59 seconds.

@NfNitLoop
Copy link

I ended up implementing my own. If someone wants to fold that into Luxon as a feature, I'll be happy to use it from there. :)

function relativeDuration(duration: Duration, opts?: Opts): string {
    let sigU = opts?.significantUnits ?? 2
    if (sigU < 1) {
        throw Error("Signficant units can't be < 1")
    }

    let units = opts?.units ?? defaultUnits
    // Make sure units are ordered in descending significance:
    units = allUnits.filter(it => units.includes(it))

    
    let negative = duration.valueOf() < 0
    if (negative) { duration = duration.negate() }
    duration = duration.shiftTo(...units)

    // Remove unnecessary most-significant units:
    while (units.length > 1) {
        if (duration.get(units[0]) > 0) {
            // we've found the most significant unit:
            break
        }

        units = units.slice(1)
        duration = duration.shiftTo(...units)
    }

    units = units.slice(0, sigU)
    duration = duration.shiftTo(...units)
    // If the last unit has fractional bits, we don't care. We're explicitly limiting significant units:
    let lastUnit = units[units.length - 1]
    duration = duration.set({
        [lastUnit]: Math.floor(duration.get(lastUnit))
    })

    let relative = duration.toHuman()
    if (negative) {
        return `${relative} ago`
    } else {
        return `in ${relative}`
    }
}

interface Opts {
    // Default: 2
    significantUnits?: number

    // Default: all but quarters & months
    units?: (keyof DurationObjectUnits)[]
}

const allUnits: ReadonlyArray<keyof DurationObjectUnits> = ["years", "quarters", "months", "weeks", "days", "hours", "minutes", "seconds", "milliseconds"]
// No quarters/weeks:
const defaultUnits: ReadonlyArray<keyof DurationObjectUnits> = ["years", "months", "days", "hours", "minutes", "seconds", "milliseconds"]

@sphinx9
Copy link

sphinx9 commented May 23, 2022

I thought specifying "multiple units" like ["minutes", "seconds"] would allow me to see output like in 1 minute 23 seconds

I also interpreted it to mean that, and I'm currently lacking a way to format a relative date in that way, so it'd be nice to see this feature added.

@steveschmitt
Copy link

At least make round mean round(), not floor().

@SeanDunford
Copy link

I think this or something similar would be a great addition. Especially when moving from moment where the default behavior was different.

const moment = require('moment') 
const luxon = require('luxon')
const halfdayPadding = 1000 * 60 * 60 * 12;
moment('2023-08-25T15:38:06.733Z').fromNow() === luxon.DateTime.fromISO('2023-08-25T15:38:06.733Z').toRelative({ round: true, padding: halfdayPadding })

Having to add this halfday padding constant doesn't seem very ergonomic.

moment('2023-08-25T15:38:06.733Z').fromNow() === luxon.DateTime.fromISO('2023-08-25T15:38:06.733Z').toRelative({ round: 'up' })

Would be much more graceful.

@corymharper
Copy link
Author

@icambron Sorry for the tag, you're just the first maintainer I encountered. I'd be willing to contribute this change myself, but considering it's a change that as described would break existing functionality (changing the boolean option to a series of strings), I didn't want just to do the work and make a pull request without having an indication of if the project owners had any interest in this change to begin with. I'm tagging you to see if I can get some feedback there, or if maybe with some tweaks such a change would be accepted.

@hugohutri
Copy link

hugohutri commented Feb 6, 2025

I am facing the same issue. The difference is 1.99 days and the format displays "in 1 day" :/

One solution is to patch the diffRelative function so that it it doesn't round towards zero (the last parameter to the roundTo function)

Something like this should do the trick:

@@ -6269,7 +6269,7 @@ function quickDT(obj, opts) {
 function diffRelative(start, end, opts) {
   var round = isUndefined(opts.round) ? true : opts.round,
     format = function format(c, unit) {
-      c = roundTo(c, round || opts.calendary ? 0 : 2, true);
+      c = roundTo(c, round || opts.calendary ? 0 : 2, false);
       var formatter = end.loc.clone(opts).relFormatter(opts);
       return formatter.format(c, unit);
     },

That option should probably be added to the opts object, eg. roundTowardZero: boolean or something

@diesieben07
Copy link
Collaborator

I would prefer upgrading the round parameter to support 'down' and 'up' in addition to the currently supported true and false. A pull request would be welcome!

@hugohutri
Copy link

hugohutri commented Feb 14, 2025

I would prefer upgrading the round parameter to support 'down' and 'up' in addition to the currently supported true and false. A pull request would be welcome!

I created a pull request for this! However, I decided to add only the "nearest" option for now, since it was very small change. And I don't know if anyone would even need the "up" / "ceil" option, but that could be added in a future pull request if someone really needs that. down and nearest are probably the only options one actually needs

@Shiva127
Copy link

Shiva127 commented Feb 19, 2025

I've made a PR (#1685) adding a new parameter rounding covering all options: towardsZero, awayFromZero, round, floor, or ceil. This new rounding option works with round being true and false.

Hope you will find it useful.

Sorry for the PR mess. 😥

@Shiva127 Shiva127 linked a pull request Feb 19, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants