Normative: Changes to AddDurationToYearMonth #3208
Conversation
Add years/months first, then skip to the end of the month if the duration sign is negative, then add the other duration fields. This is to address cases with overflow: reject where previously, an exception was thrown due to constraining the internally-constructed end-of-the-month date. In the spec, new abstract operations CalendarDateLastDayOfMonth and NonISODateLastDayOfMonth are added to make the algorithm simpler to specify.
|
The test262 failures are expected; see tc39/test262#4783 . |
|
The |
ptomato
left a comment
ptomato
left a comment
There was a problem hiding this comment.
This is much nicer than the previous approach with two overflow parameters!
(Comments on the polyfill code also apply to spec text.)
| let previousIsoDate = isoDate; | ||
| let nextIsoDate = isoDate; | ||
| let calendarDate = calendarImplForID(calendar).isoToDate(nextIsoDate, { daysInMonth: true }); | ||
| while (calendarDate.day !== calendarDate.daysInMonth) { | ||
| previousIsoDate = nextIsoDate; | ||
| calendarDate = calendarImplForID(calendar).isoToDate(nextIsoDate, { daysInMonth: true }); | ||
| try { | ||
| nextIsoDate = CalendarDateAdd(calendar, nextIsoDate, { days: 1 }, 'constrain'); | ||
| } catch (e) { | ||
| // Date overflowed maximum range => consider this the last day | ||
| break; | ||
| } | ||
| } | ||
| return previousIsoDate; |
There was a problem hiding this comment.
The previous approach for jumping to the last day of the month (jump to day 1, add 1 month, subtract 1 day) was chosen mainly because it made the fewest calls into user code. Since we dropped user-defined calendars, we will always have zero calls into user code, so I'm not opposed to rewriting this to be more efficient.
But, daysInMonth is the count of days in the month, not the day number of the last day of the month so although this rewrite is correct for all currently supported calendars, it'll break when ICU adds a calendar that skips days (which has been a plan for some time now.)
You could add days one at a time until the month number changes? or just add the number of days in the month minus 1, I think?
There was a problem hiding this comment.
I'm also not (yet) convinced about stopping the loop early when we get to the end of the range. Although it would allow Temporal.PlainYearMonth.from('+275760-09').subtract({ months: 1 }) which previously wouldn't work, it would also allow oddities like Temporal.PlainYearMonth.from('+275760-09').subtract({ weeks: 2 }) resulting in +275760-08.
There was a problem hiding this comment.
One thing we could do to not error out on the end of the range, is just skip the second addition if the weeks and days are zero.
There was a problem hiding this comment.
Changed in cf79f59 to not assume there are no skipped days in the month.
There was a problem hiding this comment.
Although it would allow Temporal.PlainYearMonth.from('+275760-09').subtract({ months: 1 }) which previously wouldn't work, it would also allow oddities like Temporal.PlainYearMonth.from('+275760-09').subtract({ weeks: 2 }) resulting in +275760-08.
If we don't stop the loop early, we get Temporal.PlainYearMonth.from('+275760-09').subtract({ weeks: 2 }) throwing because 275760-09-16 is out of range. I find that just as surprising as the result being 275760-08. I'm keeping it as it is for now, but could be convinced otherwise.
| <emu-clause id="sec-temporal-nonisodatelastdayofmonth" type="implementation-defined abstract operation"> | ||
| <h1> | ||
| NonISODateLastDayOfMonth ( | ||
| _calendar_: a calendar type that is not *"iso8601"*, | ||
| _isoYear_: an integer, | ||
| _isoMonth_: an integer between 1 and 12 (inclusive), | ||
| ): an ISO Date Record | ||
| </h1> | ||
| <dl class="header"> | ||
| <dt>description</dt> | ||
| <dd> | ||
| The operation performs implementation-defined processing to determine the last day of the month denoted by _isoDate_. It returns an ISO Date Record with the same year and month as _isoYear_ and _isoMonth_, and the day set to the last day of _isoMonth_ in _isoYear_ according to the years, months and weeks reckoning of _calendar_. | ||
| </dd> | ||
| </dl> | ||
| </emu-clause> |
There was a problem hiding this comment.
I think it'd be better to specify CalendarDateLastDayOfMonth in terms of existing implementation-defined operations if possible, then we wouldn't have to have another implementation-defined operation.
There was a problem hiding this comment.
The problem is that if we specify it in terms of NonISODateAdd, there's no way to "catch" the exception if the result of the addition is outside the representable range and we want to back off to the last representable day. (Or is there a way to catch exceptions in spec language?)
2 participants
Add years/months first, then skip to the end of the month if the duration sign is negative, then add the other duration fields. This is to address cases with overflow: reject where previously, an exception was thrown due to constraining the internally-constructed end-of-the-month date.
In the spec, new abstract operations CalendarDateLastDayOfMonth and NonISODateLastDayOfMonth are added to make the algorithm simpler to specify.
Closes #3197