moment / luxon

⏱ A library for working with dates and times in JS
https://moment.github.io/luxon
MIT License
15.32k stars 730 forks source link

splitBy({weeks: 1}) does not accurately split at the end of the week #1542

Closed pano9000 closed 10 months ago

pano9000 commented 10 months ago

Describe the bug When trying to split an interval by weeks, it seems that luxon does not accurately split at the end of the week, as one would expect it.

To Reproduce

const firstWeekInYear = luxon.DateTime.fromObject({weekYear: 2023, weekNumber: 1}, {zone: "UTC"});
const lastWeekInYear = luxon.DateTime.fromObject({weekYear: 2023, weekNumber: firstWeekInYear.weeksInWeekYear}, {zone: "UTC"});
const intervalYear =   firstWeekInYear.until(lastWeekInYear.endOf("week"));

This correctly produces the expected Interval { start: 2023-01-02T00:00:00.000Z, end: 2023-12-31T23:59:59.999Z }, where the week starts at 00:00:00 and ends at 23:59:59.999Z on the last day of the week.

I then try to split the interval by weeks like so:

const intervalWeeks = intervalYear.splitBy({weeks: 1});

This produces the following example output:

Interval { start: 2023-01-02T00:00:00.000Z, end: 2023-01-09T00:00:00.000Z },
Interval { start: 2023-01-09T00:00:00.000Z, end: 2023-01-16T00:00:00.000Z },
[...]
Interval { start: 2023-12-18T00:00:00.000Z, end: 2023-12-25T00:00:00.000Z },
Interval { start: 2023-12-25T00:00:00.000Z, end: 2023-12-31T23:59:59.999Z }

Here the end date of the week is actually the first date of the following week, only the very last date is correctly displayed (because it is based on the value from .endOf("week")).

Actual vs Expected behavior I would expect the end date to be showing like the following instead:

Interval { start: 2023-01-02T00:00:00.000Z, end: 2023-01-08T23:59:59.999Z },
Interval { start: 2023-01-09T00:00:00.000Z, end: 2023-01-15T23:59:59.999Z },
[...]
Interval { start: 2023-12-18T00:00:00.000Z, end: 2023-12-24T23:59:59.999Z },
Interval { start: 2023-12-25T00:00:00.000Z, end: 2023-12-31T23:59:59.999Z }

Not sure, if this is maybe intended behaviour or not (for some reason), so please correct me, if I am wrong.

Desktop (please complete the following information):

Additional context Add any other context about the problem here.

diesieben07 commented 10 months ago

The end of an interval is exclusive, so this is exactly what you'd expect. Interval { start: 2023-01-02T00:00:00.000Z, end: 2023-01-08T23:59:59.999Z } would mean that the last millisecond of the week is not included in the interval. In fact your intervalYear should be constructed like so:

const firstWeekInYear = luxon.DateTime.fromObject({weekYear: 2023, weekNumber: 1}, {zone: "UTC"});
const intervalYear =   firstWeekInYear.until(firstWeekInYear.plus({ years: 1 }));
pano9000 commented 10 months ago

thanks for the information, I did not know that it is exclusive - it does feels a bit odd to me, but nothing that can not be worked around with – as long as one knows about it :-)

I could not find any mention of the interval end being exclusive or inclusive (or as https://github.com/moment/luxon/issues/1105#issuecomment-1049056468 mentions it: "half-open") or anything like it in the docs - so maybe it would be a good idea to mention this in there as well?

FYI: for my use case I just used the same "trick" described in #1105

diesieben07 commented 10 months ago

The documentation for Interval says:

An Interval object represents a half-open interval of time

There is no dedicated "prose" documentation for interval, only the API docs generated from JS docs. I am sure we could improve there.

it does feels a bit odd to me

Trust me, you don't want closed intervals. They make most math operations harder.