Regression: Duration.normalize() not working properly after 3.4.1

[thomassth]

Describe the bug
See https://github.com/moment/luxon/actions/runs/5943754818/job/16119553727

- Expected  - 2
+ Received  + 2

  Object {
-   "days": 363,
-   "years": 1,
+   "days": -2,
+   "years": 2,
  }

- Expected  - 2
+ Received  + 2

  Object {
-   "days": 2,
-   "months": 0,
+   "days": -28,
+   "months": 1,
  }

Likely the fix for #1482 in 3.4.1 (11e454c) broke #1467

To Reproduce
Duration.fromObject({ hours: 96, minutes: 0, seconds: -10 }).normalize().toObject())
should equal { hours: 95, minutes: 59, seconds: 50} but no conversion happened.

Actual vs Expected behavior
All tests should be passing

[tonysamperi]

Exactly, just wanted to report the same! I discovered it while updating my ts-luxon.
I don't mean to be rude, but what's the point of having tests if you don't run them before publishing?
Anyway I can try to figure out a way to make both

"Duration#normalize rebalances negative units"
and the newer
"Duration#shiftTo handles mixed units"

if you confirm that both can pass at the same time.

Cheers

[thomassth]

In a way we can just run rescale() before every normalize(), since rescale never had any problem on conversion

But that feels wrong

[tonysamperi]

@thomassth no we can't rescale, because rescale calls normalize 😅

[diesieben07]

I apologize for the broken release again.
I'm currently sitting down with pen and paper to revise the normalization algorithm once and for all. It has caused nothing but trouble in the past, resulting in numerous issues like this.

[tonysamperi]

@diesieben07 is there an analysis of all the use cases the algorythm should cover?
Because I'm really intrigued by the problem...yesterday I spent several hours, but when I solved one, I broke another.
Also I'm having trouble to understand if the problem are negative units, or adjustments that happen between distant units (e.g year / day) which have then a very small raw value...

[diesieben07]

Here is (as far as I see it) what normalize should do (assuming the overall value of the duration is positive):

• Remove excessive "lower order" values, by converting them to higher order units, e.g. { years: 2, days: 400 } becomes { years: 3, days: 35 }
• Remove negative "lower order" values, by reducing a higher order unit to make the lower order positive, e.g. { years: 2, days: -2 } becomes { years: 1, days: 363 }.

The previous implementations never really made this distinction clear and attempted to handle both cases in unison.
I have now implemented two code paths, one for each case. All tests now pass, but I am going to investigate further if all cases are covered by the current test suite before committing to a pull request.

As for units which are overall negative in value, they are handled the same way, except that every step is inverted. For example { years: -2, days: 2 } becomes { years: -1, days: -363 }.

[tonysamperi]

Ok that makes sense. And I saw in the code the handling of overall negative values, managed with a double negate. That should be fine as long as the 2 cases above are handled correctly.

[diesieben07]

And I saw in the code the handling of overall negative values, managed with a double negate.

Correct. Although I have improved this now, removing the need for multiple intermediate objects.