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#normalize has trouble with negative milliseconds #1233

Closed
elliott-king opened this issue Jul 8, 2022 · 6 comments · Fixed by #1467
Closed

Duration#normalize has trouble with negative milliseconds #1233

elliott-king opened this issue Jul 8, 2022 · 6 comments · Fixed by #1467

Comments

@elliott-king
Copy link
Contributor

elliott-king commented Jul 8, 2022

Describe the bug
Fails to normalize a Duration with negative milliseconds.

To Reproduce
I was able to repro this in the playground in the docs:

> luxon.VERSION
'2.4.0'

> let d = luxon.Duration.fromObject({seconds: -44, milliseconds: -100, minutes: 1})

> d.normalize()
Duration {values: {}, loc: Locale, conversionAccuracy: 'casual', invalid: null, matrix: {},}
...
values: {seconds: 16, milliseconds: -100, minutes: 0}

I would expect the above to be {seconds: 15, milliseconds: 900, minutes: 0}

Out of curiosity, I tried the initializing the "right" duration, and it has no issues normalizing:

> let m = luxon.Duration.fromObject({seconds: 15, milliseconds: 900})

> m.normalize()
Duration {values: {}, loc: Locale, conversionAccuracy: 'casual', invalid: null, matrix: {},}
...
values: {seconds: 15, milliseconds: 900}

Actual vs Expected behavior
Since the overall duration is greater than zero, I expect all normalized fields to be greater than zero.

Desktop (please complete the following information):

  • OS: MacOS 11.6.5
  • Browser: Chrome Version 102.0.5005.115 (Official Build) (x86_64)
  • Luxon version: 2.4.0 (used your playground)
  • Your timezone "America/New_York"

Additional context
This library is excellent, by the way!

@elliott-king
Copy link
Contributor Author

Possibly unrelated: I would also expect normalize to change 66 seconds into a combination of seconds & minutes:

> luxon.Duration.fromObject({days: 777}).normalize()
...
values: {days: 777}

> luxon.Duration.fromObject({milliseconds: 15900}).normalize()
...
values: {milliseconds: 15900}

@icambron
Copy link
Member

icambron commented Jul 9, 2022

Yeah, that could definitely use some improvements

@CristiMacovei
Copy link

@icambron Can I solve this issue?

@icambron
Copy link
Member

icambron commented Oct 3, 2022

@CristiMacovei sure

@ZainGulbaz
Copy link

@icambron Can you please assign this to me? Thanks.

thomassth added a commit to thomassth/luxon that referenced this issue Jun 29, 2023
thomassth added a commit to thomassth/luxon that referenced this issue Jun 29, 2023
@thomassth
Copy link
Contributor

Hi @icambron , would you mind checking out #1467 ? I've included a test that'd fail in current builds, but passing after PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants