luxon icon indicating copy to clipboard operation
luxon copied to clipboard

Bugs with DateTime#toRelative padding

Open arubacao opened this issue 5 years ago • 3 comments

Hi guys,

I've noticed an issue with toRelative with the padding option. Before I try to explain anything, I simply added some tests to illustrate the issue best:

  // works as expected
  expect(base.plus({ minutes: 1, seconds: 30 }).toRelative({ base, padding: 30000 })).toBe(
    "in 2 minutes"
  );
 // doesn't work
  expect(base.plus({ seconds: 29 }).toRelative({ base, padding: 30000 })).toBe("in 29 seconds");

Test result:

  ● DateTime#toRelative allows padding

    expect(received).toBe(expected) // Object.is equality

    Expected: "in 29 seconds"
    Received: "in 59 seconds"

      39 |     "in 2 minutes"
      40 |   );
    > 41 |   expect(base.plus({ seconds: 29 }).toRelative({ base, padding: 30000 })).toBe("in 29 seconds");
         |                                                                           ^
      42 | });

arubacao avatar Oct 12 '20 21:10 arubacao

This seems to be a common misconception around this padding option, which is not meant to round or align the results. See for instance #745 for details.

The documentation should be improved, or even maybe the name of this option changed.

gdebunne avatar Oct 14 '20 19:10 gdebunne

Thanks for the clarification @gdebunne ! IMO this feature would still be great in this library. Here is a usecase:

A user get's 2 minutes time for a task. A timer is displayed along the task from 2mins to 0 seconds. Using toRelative without extra options, the timer starts displaying "in 1 minute" immediately, while the more natural wording to me would be "in 2 minutes"

Since I was explicitly searching for this, I was really exited about padding but obvs. misunderstood it.

arubacao avatar Oct 15 '20 08:10 arubacao

Yes, @gdebunne is correct; the padding option is to fix a specific problem with drift, not this.

This is certainly a valid feature request, though.

icambron avatar Oct 23 '20 01:10 icambron