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

Duration.fromObject({ hours: 2 }).milliseconds is 0 #1626

Closed
KlausNie opened this issue May 15, 2024 · 4 comments
Closed

Duration.fromObject({ hours: 2 }).milliseconds is 0 #1626

KlausNie opened this issue May 15, 2024 · 4 comments

Comments

@KlausNie
Copy link

Describe the bug
Duration.fromObject({ hours: 2 }).milliseconds === 0

Why does it not behave like

To Reproduce

import {
  DateTime,
  Duration
} from "luxon";

describe(`test`, function () {
  it(`should work`, function () {
    // correct behaviour with Duration created by diff
    const d1 = DateTime.now().minus({ days: 1 });
    const d2 = DateTime.now().plus({ days: 2 });
    const dur1 = d2.diff(d1);
    const diffInMilliseconds = dur1.milliseconds;
    console.log(diffInMilliseconds);
    expect(diffInMilliseconds).not.toBe(0);

    // incorrect behaviour with Duration creatd by Duration factory
    const duration = Duration.fromObject({ hours: 2 });
    console.log(duration.milliseconds);
    expect(duration.milliseconds).not.toBe(0)
  });
});

Actual vs Expected behavior
Durations created by factories should behave the same way as diff Durations
OR
this needs to be documented!!!

Desktop (please complete the following information):

  • luxon 3.4.4

Additional context
IMO, the convenience accessors like get milliseconds(): IfValid<number, typeof NaN, IsValid>; are not fit for the Duration interface. It makes sense, for DateTime to get:

  • milliseconds: the milliseconds part of this date
  • date as milliseconds: time from 1970 as milliseconds
    but not for the Duration interface.
@diesieben07
Copy link
Collaborator

What do you expect Duration.fromObject({ years: 2 }).days to return? Valid options are 730, 731, 732. Which one is correct depends on your use case, because some years are leap years and some are not.
Conversions between raw duration units (not bound to a particular point in time) are going to be lossy hence Luxon does not do them by default.

This is documented extensively: https://moment.github.io/luxon/#/math?id=duration-math.

The reason diff behaves "like you expect" is because you omitted the optional parameter unit that tells Luxon which units you want in the result. The default is "milliseconds", so because you didn't specify this parameter you get a Duration with just milliseconds in it. If you were to look at e.g. hours of the resulting Duration it would be 0, because you did not ask for hours.

@KlausNie
Copy link
Author

@diesieben07
Understandable and makes sense.
It wouldn't hurt to add that information to

    /**
     * Get the years.
     */
    get years(): IfValid<number, typeof NaN, IsValid>;

    /**
     * Get the quarters.
     */
    get quarters(): IfValid<number, typeof NaN, IsValid>;

    /**
     * Get the months.
     */
    get months(): IfValid<number, typeof NaN, IsValid>;

    /**
     * Get the weeks
     */
    get weeks(): IfValid<number, typeof NaN, IsValid>;

    /**
     * Get the days.
     */
    get days(): IfValid<number, typeof NaN, IsValid>;

    /**
     * Get the hours.
     */
    get hours(): IfValid<number, typeof NaN, IsValid>;

    /**
     * Get the minutes.
     */
    get minutes(): IfValid<number, typeof NaN, IsValid>;

    /**
     * Get the seconds.
     */
    get seconds(): IfValid<number, typeof NaN, IsValid>;

    /**
     * Get the milliseconds.
     */
    get milliseconds(): IfValid<number, typeof NaN, IsValid>;

in the code, or just that documentation link you posted. Unless I'm the only one who stumbled upon that, then you can ignore my suggestion.

@diesieben07
Copy link
Collaborator

The Duration class itself does talk about this, although it could be a bit clearer maybe.
Unfortunately I do not really have the time (and energy) to work on Luxon at the moment (as you can see from the backlog of issues...), but if you think you can improve the inline documentation, a PR would be welcome.

@KlausNie
Copy link
Author

@diesieben07
let me know what you think: #1627

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

No branches or pull requests

2 participants