Closed VaheAntonyan closed 1 year ago
This is a bug of sorts, but it's a subtle one. At some level it's a question of exactly what diffing across months and years really means. I certainly understand the intuition behind the 3 days: namely that 2021-04-01 is 3 days aster 2021-03-29, and it seems reasonable to interpret 03-29 and as a 1 year, 1 month after 02-29.
However, that last part isn't how Luxon does the diff. It adds the years first, then the months, then the days, figuring out at each step how much it has to add. And the problem is the year part: 2020-02-29 + 1 year isn't 2021-02-29 because that day doesn't exist. Instead it's 2021-02-28. A month after that it's 2021-03-28, and the 4 days after that it's 2021-04-01.
DateTime.fromISO("2020-02-29").plus({ years: 1 }).plus({ months: 1 }).plus({ days: 4 }).toISO()
//=> '2021-04-01T00:00:00.000-04:00'
You could argue that Luxon should sort of combine the year and month calculations so that it doesn't decide the day in the new year until after it's figured out the months part, instead of deciding right after adding the year that the day is X and then continuing on. And there's precedence for that in Luxon. For example, if we do the add all at once instead of piecemeal as above, Luxon does exactly that:
DateTime.fromISO("2020-02-29").plus({ years: 1, months: 1, days: 4 }).toISO()
//=> '2021-04-02T00:00:00.000-04:00'
And I do think that whatever conventions Luxon uses, we should be consistent between what diff
produces and what plus
computes. In other words:
const someDiff = dt1.diff(dt2)
const addedBack = dt1.plus(someDiff)
addedBack.equals(dt1) // => should be true
That's being violated here, so we should fix diff
to be consistent. So I'd take a PR for this as long as it wasn't like a ton of code.
And I do think that whatever conventions Luxon uses, we should be consistent between what
diff
produces and whatplus
computes. In other words:const someDiff = dt1.diff(dt2) const addedBack = dt1.plus(someDiff) addedBack.equals(dt1) // => should be true
That's being violated here, so we should fix
diff
to be consistent. So I'd take a PR for this as long as it wasn't like a ton of code.
const someDiff = dt1.diff(dt2)
const addedBack = dt2.plus(someDiff) // typo, dt2 instead of dt1
addedBack.equals(dt1) // => should be true
Actually diff
and plus
are consistent in this case.
I think that
dt1.plus({ years: x, months: y, days: z })
should be same, as
dt1.plus({ years: x }).plus({ months: y }).plus({ days: z })
but it is more like
dt1.plus({ months: y }).plus({ years: x }).plus({ days: z })
Consistency of going from bigger unit to smaller one is lost here, like we have for months and days case.
dt1.plus({ months: x, days: y })
dt1.plus({ months: x }).plus({ days: y })
Actually diff and plus are consistent in this case.
They aren't:
DateTime.fromISO("2020-02-29").plus({ years: 1, months: 1, days: 3 }).toISO()
//=> '2021-04-01T00:00:00.000-04:00'
DateTime.fromISO("2021-04-01").diff(DateTime.fromISO("2020-02-29"), ["years", "months", "days"]).toObject()
//=> {years: 1, months: 1, days: 4}
So 3 days vs 4 days, same spans of time. (If you add 4 days instead, you get April 2.) I think that's the core of the issue here.
I think that...should be same, as
I don't think that's right, and would violate a lot of expectations. Moreover, it would violate your expectations. Your original issue is that you expect your diff to give you 3 days, not 4. That's expecting Luxon to handle end-of-month logic first and then add the years, which is consistent with the current behavior of plus()
but not with the current behavior of diff()
. Doing years then months then days is exactly what diff()
does, which runs into "2021-02-29 doesn't exist" problems.
Or put another way, this is consistent with diff and apparently not what you want:
DateTime.fromISO("2020-02-29").plus({ years: 1}).plus({ months: 1}).plus({ days: 3 }).toISO() // => '2021-03-31T00:00:00.000-04:00'
I agree with original you and disagree with new you.
Yes, you are right, my 2nd comment is wrong, thank you.
Here's a test case for this:
// see https://github.com/moment/luxon/issues/1301
test("DateTime#diff handles Feb-29 edge case logic for higher order units in a manner consistent with DateTime#plus", () => {
const left = DateTime.fromISO("2020-02-29");
const right = DateTime.fromISO("2021-04-01");
const diff = right.diff(left, ["year", "month", "day"]);
expect(diff.days).toBe(3);
expect(left.plus(diff).equals(right)).toBe(true);
});
I haven't really wrapped my head around the exact nature of this bug and the current diff algorithm properly yet, but I notice that these dates also span DST and wonder if that implied change in offset may be playing a role as well. I am finding it challenging to generalize this bug with other dates that are problematic in a similar way, but I will try to puzzle it out and will post a PR if I am successful.
@dobon thanks for the help here.
It's not a very general bug. The best way to think of it is, "what is 2020-02-29 plus 1 year, 1 month"?
.plus({years: 1}).plus({ months: 1})
it comes up with 03-28 where you expect 03-29.plus({years: 1, months: 1})
does, though it uses a sort of oblique technique to pull that offIn other words, it's very specific to leap years and ends of months. I don't think DST plays any part.
The way diff works is that it starts adding units one at a time. It takes the units in order and just adds as much of that unit as it can. So like .diff(..., ["years", "months", "days"])
adds as many years as it can, then as many months as it can, then as many days as it can (it does the "as much as it can" part with a pretty naive subtraction called "differ" in the code, then subtracts one back off if it overshoots). There are complications there having to do with what you do with remainders, but that's the gist of it.
So the problem is really that it does it one unit at a time, and those are complete additions, as in literal plus({ /* one unit and value */ })
calls. So it's just like the first method of addition above, and it never gets a chance to do the second method.
Edit: it's worth looking at how plus()
actually does this. It's not particularly clever, but it might lead to the right technique to use in diff
. I think what diff needs to do is probably something like this:
That way, if we accumulated 1 year, we think the high water is { year: 2021, month: 02, day: 29 }
which is invalid, but we don't do anything about it, just keep going. That should let us get to 03-29. But I haven't actually tried it and it might be more complicated than that.
@icambron Here's my PR: https://github.com/moment/luxon/pull/1340
The key change I made was to avoid using an intermediate "cursor" DateTime, and instead build up a duration-like hash which is always directly added to the 'earlier' DateTime and compared against the 'later'. This way we are directly using the hash-based DateTime#plus method when creating the diff Duration, thus it is guaranteed to be consistent.
Please let me know what you think about this solution, when you have time to review.
@icambron I've cleaned up my PR so that there are now very simple and minimal changes from current algorithm. Actually, I'm surprised how clean the patch turned out, lol. It should be easy for you to review now. Cheers.
This issue can be closed now. Fix was published in 3.2.0
Wrong diff calculation
To Reproduce
Actual vs Expected behavior Actual behavior: //=> {years: 1, months: 1, days: 4} Expected behavior: //=> {years: 1, months: 1, days: 3}