shiftAll() regression

[Maxim-Mazurok]

Describe the bug What used to be P99Y11M3W2DT10H29M6S in v3.3.0 is now P99Y11M3.3481250000000005W in v3.4.2

To Reproduce

const { DateTime, Duration } = require("luxon@3.4.2");

const purchaseDate = DateTime.fromObject(
  { day: 1, month: 1, year: 2020 },
  { zone: "UTC", conversionAccuracy: "longterm" }
);
const warranty = Duration.fromObject({ years: 100 }, { zone: "UTC", conversionAccuracy: "longterm" });
const now = DateTime.fromObject(
  { day: 8, month: 1, year: 2020 },
  { zone: "UTC", conversionAccuracy: "longterm" }
);
const age = now.diff(purchaseDate).shiftToAll();
const warrantyLeft = warranty.minus(age).shiftToAll();

console.log({
  warrantyLeft: warrantyLeft.toString(),
});

You can run it on https://npm.runkit.com/luxon and see the unexpected {warrantyLeft: "P99Y11M3.3481250000000005W"} in the output.

Actual vs Expected behavior

const { DateTime, Duration } = require("luxon@3.3.0");

const purchaseDate = DateTime.fromObject(
  { day: 1, month: 1, year: 2020 },
  { zone: "UTC", conversionAccuracy: "longterm" }
);
const warranty = Duration.fromObject({ years: 100 }, { zone: "UTC", conversionAccuracy: "longterm" });
const now = DateTime.fromObject(
  { day: 8, month: 1, year: 2020 },
  { zone: "UTC", conversionAccuracy: "longterm" }
);
const age = now.diff(purchaseDate).shiftToAll();
const warrantyLeft = warranty.minus(age).shiftToAll();

console.log({
  warrantyLeft: warrantyLeft.toString(),
});

You can run it on https://npm.runkit.com/luxon and see the expected {warrantyLeft: "P99Y11M3W2DT10H29M6S"} in the output.

Desktop (please complete the following information):

• OS: WSL2 in Windows
• Browser N/A
• Luxon version 3.4.2
• Your timezone Australia/Sydney

Additional context Might be loosely related to https://github.com/moment/luxon/issues/1482 ?

[diesieben07]

Sorry again for the regression, this one is on me. This case is unfortunately not covered by the current tests, so it was not caught. I was under the impression we had enough tests, so I was confident in my solution. Unfortunately this was not true. I am investigating this issue at the moment. The problem is, again, with normalize (which shiftToAll uses). Here is a simplified reproduction:

const duration = Duration.fromObject({years: 100, months: 0, weeks: -1, days: 0}, { conversionAccuracy: 'longterm' });
console.log(duration.normalize().toObject());

[Maxim-Mazurok]

All good, this is why I have tests :) I'm glad they helped to catch these things! I'm in no hurry to upgrade, v3.3.0 seems to work fine for my needs, I've put a note for myself to try updating again when this gets resolved, cheers!

[Maxim-Mazurok]

Also wanted to mention that doing bla.shiftToAll().shiftToAll() produces desired/expected results, maybe that will help

[diesieben07]

@Maxim-Mazurok Please see the pull request I have created. Additionally, I have noticed that in your code you are passing the option conversionAccuracy to the DateTime factory methods. Those methods do not have such an option, so it has no effect here.

[Maxim-Mazurok]

Thank you, the fix works!