Fix isoweek startOf returning incorrect date for the first day of the week.

Billiam commented 3 years ago

This PR fixes an issue causing days that land on the isoweek day to round down to the previous week.

I've added a spec that fails with the previous code.

stockiNail commented 3 years ago

@Billiam I have some doubts about this PR, maybe it is my lack of knowledge about calendar.

Let's say we want to know the start of the week for date 22nd July (yesterday) and we decide the Saturday is start of week.. Date: 7/22/2021 WeekDay: 4 (Thu) ISOWeekDay: 6 (Sat)

const weekday = 6;  
const dateTime = luxon.DateTime.fromObject({day: 22, hour: 8, minute: 30});
const startOfWeekDateTime = dateTime.minus({days: (dateTime.weekday % 7) - weekday}).startOf('day');
console.log("Date:" + dateTime.toFormat('D')+", startOfWeek: "+startOfWeekDateTime.toFormat('D'));

Result: "Date: 7/22/2021, startOfWeek: 7/24/2021"

Sounds strange to me that a date belongs a week where the starting day of that week is after the date itself. Instead, with the current logic:

const weekday = 6;  
const dateTime = luxon.DateTime.fromObject({day: 22, hour: 8, minute: 30});
const startOfWeekDateTime = dateTime.minus({days: (dateTime.weekday < weekday ? 7 : 0) + dateTime.weekday - weekday}).startOf('day');
console.log("Date:" + dateTime.toFormat('D')+", startOfWeek: "+startOfWeekDateTime.toFormat('D'));

Result: "Date:7/22/2021, startOfWeek: 7/17/2021"

Nevertheless it sounds to be an error when the weekday of date is Sunday (0) and ISOweekday is Sunday (0) as well.

Test with Sunday, 18th July.

const weekday = 0;  
const dateTime = luxon.DateTime.fromObject({day: 18, hour: 8, minute: 30});
const startOfWeekDateTime = dateTime.minus({days: (dateTime.weekday < weekday ? 7 : 0) + dateTime.weekday - weekday}).startOf('day');
console.log("Date:" + dateTime.toFormat('D')+", startOfWeek: "+startOfWeekDateTime.toFormat('D'));

Result: "Date:7/18/2021, startOfWeek: 7/11/2021"

Billiam commented 3 years ago

Whoops! I thought I'd checked for that, but I was leaning on the tests (which were passing!) without verifying them.

I've updated the date math with @kurkle's suggestion, and have fixed three issues in the tests that allowed it to pass:

expect(startOf.day).not.toBeGreaterThan(dt.day);

startOf is in milliseconds, not a DateTime object so startOf.day returns undefined
undefined is not greaterThan the integer dt.day, so the test passes
even if it was working, comparing day for those two values is incorrect, since it would return false negatives and positives when the dates fell in two different months.