Fix Duration#normalize, shiftTo and shiftToAll

[diesieben07]

This fixes #1496 by introducing an additional conversion step in Duration#normalize / normalizeValues.
This step converts any fractions in larger units into lower units, if possible.
For example: { years: 2.5, days: 0, hours: 0 } becomes { years: 2, days: 182, hours: 12 } (assuming casual conversions).

This also affects shiftTo and shiftToAll. I have added tests for all three methods to test this behavior.

Note: This alters the behavior of normalize, because previously normalize would not do this. {years: 2.5, days: 0} would stay as-is. I would argue that that is a bug however, which is fixed by this pull request. shiftTo (and by extension shiftToAll) did do this, prior to calling normalize, however I would argue that this is something that should be handled by normalize.

[icambron]

Moving fractional parts of bigger units into smaller units is totally something normalize() should do

[jayaddison]

A couple of questions about this:

• If the fraction of the larger unit would result in a fractional (non-decimal) number of the smaller units, will the smaller unit still be adjusted?
• Could this make normalize non-idempotent? (as in, normalize(normalize(n)) != normalize(n))
• Maybe a question about normalize more generally (I don't see it in the documentation currently) - is there a way to configure the conversion matrices used during normalization? Years and months in particular seem like they can be ambiguous in duration.

[diesieben07]

@jayaddison

If the fraction of the larger unit would result in a fractional (non-decimal) number of the smaller units, will the smaller unit still be adjusted?

Yes, fractions will be moved down as much as possible. Or, worded differently, only the smallest unit should have fractions after normalization.

Could this make normalize non-idempotent? (as in, normalize(normalize(n)) != normalize(n))

Not as far as I can tell, because the 2nd call would not find any fractions to move to smaller units. Do you have a situation in mind where this could happen?

Maybe a question about normalize more generally (I don't see it in the documentation currently) - is there a way to configure the conversion matrices used during normalization? Years and months in particular seem like they can be ambiguous in duration.

The matrix can be configured on the Duration itself and is used for more than just normalization. You can find out more in the documentation.

[jayaddison]

Thanks very much @diesieben07 - that's reassuring, and resolves all my questions. Also my apologies - I didn't find that section of the documentation. It's an enjoyable read!

The concern I had about a non-idempotent case was not a realistic one as long as the normalization is a strictly a single-pass greatest-to-smallest: I had wondered about, for example, fractional years being shifted down to days, but that resulting in a value that is above a month-or-week threshold, and therefore a subsequent call translating those days 'up' into a larger unit. It sounds like that cannot happen.

[jayaddison]

Hold on, maybe my concern returns due to this in the API docs:

excessive values for lower-order units are converted to higher-order units (if possible, see first and second example)

As usual I should read and test more before commenting. I will take more time to look into this soon.

So conversion 'up' could occur?

[diesieben07]

Yes, conversion up will occur, but only for parts of lower order units that wholly fit into higher order units.
For example { years: 0, months: 13 } will become { years: 1, months: 1 }.

This cannot be in conflict with the first part, because we only convert fractional parts of the higher order units downwards. This means that those parts will never be considered for upward conversion, because they would not fit into an integer part of the higher unit.

Example:
In {years: 0.5, months: 0} we convert the 0.5 years into 6 months. Those 6 months will never be considered for upwards conversion, because they would not turn into a whole year.

[jayaddison]

@diesieben07 Ok; experimenting with this has reassured me but I did find one more caveat:

• The good: I now understand that both 'upwards' and 'downwards' normalization do not intoduce any new unit fields into an existing Duration object. So even if a duration has years: 1, days: 7, no weeks units would be introduced. That's a helpful constraint in this context that makes reasoning about this change easier.

• The caveat: if the 'downwards' normalization of a fractional value into the following unit causes the smaller unit to rollover the threshold at which it becomes greater-than-one of the larger unit, then normalization (in terms of the .values) is non-idempotent. It's probably easier to explain with an example:

> Duration = require('luxon').Duration;
> Duration.fromISO('P2.5Y11M').normalize().values
{ years: 2, months: 17 }
> Duration.fromISO('P2.5Y11M').normalize().normalize().values
{ years: 3, months: 5 }
> Duration.fromISO('P2.5Y11M').normalize().equals(Duration.fromISO('P2.5Y11M').normalize())
true
> Duration.fromISO('P2.5Y11M').normalize().equals(Duration.fromISO('P2.5Y11M').normalize().normalize())
false

I understand that the equals check requires the same units and values -- it's not a duration-equivalence check -- despite that, this behaviour could be confusing in some situations.