Normalizing a Duration larger than one month has an unexpected amount of days

[johannaljunggren]

Describe the bug I have created a duration that is 28 days long. After I normalize it it has the value 1 month. When I get the value of the normalized duration in days it returns 30 days.

To Reproduce https://codesandbox.io/s/luxon-duration-fx9v3l

Actual vs Expected behavior const duration = Duration.fromObject({ years: 0, months: 0, weeks: 0, days: 28, hours: 0, minutes: 0, seconds: 0, milliseconds: 0 });

const normalizedDuration = duration.normalize(); Expected: normalizedDuration.as('days') == 28; Actual: normalizedDuration.as('days') == 30;

Desktop (please complete the following information):

• OS: [Windows 10]
• Browser [Firefox 117.0.1]
• Luxon version [3.4.3]
• Your timezone [CEST/GMT+2]

[icambron]

Yeah, that's a bug. The underlying issue is that it normalized 28 days to 1 month, which isn't correct. Then when you converted it back to days, 1 month turned into 30 days, which by itself is correct.

What I suspect is happening is that because you have a weeks key, 28 days gets converted to 4 weeks, like this:

Duration.fromObject({ weeks: 0, days: 28 }).normalize().toObject()   //=> { weeks: 4, days: 0 }

Then, because you have a months key too, it says "well, and 4 weeks is 1 month":

Duration.fromObject({ months: 0, weeks: 0, days: 28 }).normalize().toObject()  //=> { months: 1, weeks: 0, days: 0 }

If you leave out months, it becomes 4 weeks, 0 days, which will convert back to 28 days when you do as("days"). And if you leave out weeks, it won't be a full month, so it'll act normally:

Duration.fromObject({ months: 0, days: 28 }).normalize().toObject()  //=> { months: 0, days: 28 }

So it's the combo of months and weeks as keys in the duration that's trigging this.

Fundamentally, it's the recursive rollup that's problematic, because the casual conversion rules aren't self-consistent; it does this only because the normalization "passes through" weeks, and 4 weeks = 1 month is inconsistent with 30 days = 1 month. I think we need to improve this. @diesieben07 I know you were recently looking at duration normalization stuff. Do you have thoughts on this one? My off-the-cuff thought is that we shouldn't always be rolling up already-partially-rolled-up units, but I'm not quite sure how to formalize that.

Edited to add, since I'll forget all my thoughts on this if I don't write them down:

1. I think we might just be doing things backwards. Perhaps we need to roll up overflowed units to the biggest unit and then work our way down. So like { years: 0, months: 0, weeks: 0, days: 600 } goes into years first and then the leftovers into months, etc.
2. One nice thing is that we could handle fractional parts of bigger units needing "pushed down" into the smaller units in the same loop
3. Specifically, here this would give you 28 days, because we'd try months first, decide it's not big enough to be a month, and then do weeks, which would give us 4.
4. Compare to 37 days. There we'd get { months: 1, weeks: 1, days: 0 }. Which seems good too. Currently that produces { months: 1, weeks: 1, days: 2 } because 37 becomes 5 weeks and 2 days, and then the 4 weeks turns into a month, leaving a week and two days left over. But that's really confusing!
5. I am probably overlooking some complications here

[danganiev-tt]

It seems that this issue was introduced while fixing normalize for negative values after version 3.4.0 changes, because unit tests in the project I'm working on, which uses Duration in combination with normalize, are passing for Luxon version 3.3.0