Normative: Constrain internal days automatically in PlainYearMonth subtraction #3198
Conversation
ptomato
left a comment
There was a problem hiding this comment.
This seems like it's going to be a difficult-to-deal-with PR any way we slice it, but here are some suggestions that may make it at least a bit smaller and easier.
Also check the test262 and snapshot test results — are these legitimate failures in the sense that the previous behaviour did actually already have test coverage and those tests need to be updated, or are they regressions?
It occurred to me that a simpler approach to implementing this might be to introduce a CalendarYearMonthAdd similar to but simpler than CalendarDateAdd. That could go either way, it might result in a bunch of duplication.
|
|
||
| export function RegulateISODate(year, month, day, overflow) { | ||
| switch (overflow) { | ||
| case 'reject': | ||
| RejectISODate(year, month, day); | ||
| break; | ||
| case 'constrain': | ||
| ({ year, month, day } = ConstrainISODate(year, month, day)); | ||
| break; | ||
| } | ||
| return { year, month, day }; | ||
| export function RegulateISODate(year, month, day, overflowMonths, overflowDays) { | ||
| RejectISODate(year, month, day, overflowMonths, overflowDays); | ||
| return ConstrainISODate(year, month, day); | ||
| } |
There was a problem hiding this comment.
For this function, we don't have to make the distinction between the two types of overflow, because they don't matter in the ISO calendar. So RegulateISODate can just be left as is.
There was a problem hiding this comment.
It needs to take these arguments because they have to be passed to RejectISODate (see below).
| export function RejectISODate(year, month, day) { | ||
| RejectToRange(month, 1, 12); | ||
| RejectToRange(day, 1, ISODaysInMonth(year, month)); | ||
| export function RejectISODate(year, month, day, overflowMonths, overflowDays) { | ||
| if (overflowMonths === 'reject') { | ||
| RejectToRange(month, 1, 12); | ||
| } | ||
| if (overflowDays === 'reject') { | ||
| RejectToRange(day, 1, ISODaysInMonth(year, month)); | ||
| } |
There was a problem hiding this comment.
Likewise, RejectISODate can just stay as is.
There was a problem hiding this comment.
It doesn't work with the implementation of RejectISODate being as it is, because there's no way for it to distinguish between the case where it should (effectively) constrain the day despite overflow being reject, and the case where it should reject the day, without taking an overflow argument.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3198 +/- ##
=======================================
Coverage 96.74% 96.74%
=======================================
Files 22 22
Lines 10348 10415 +67
Branches 1859 1871 +12
=======================================
+ Hits 10011 10076 +65
- Misses 287 288 +1
- Partials 50 51 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was one test262 failure, which was a regression that I fixed in 21c8063, and no snapshot test failures. |
@ptomato I started, but I think it's going to be much more duplication, specifically for the non-ISO-calendar code. Either an extra argument needs to get passed around to permit |
See #3197