-
Notifications
You must be signed in to change notification settings - Fork 172
Editorial: Rewrite CalendarDateUntil to be simpler and more general #3136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3136 +/- ##
=======================================
Coverage 96.85% 96.85%
=======================================
Files 21 21
Lines 9983 9983
Branches 1829 1829
=======================================
Hits 9669 9669
Misses 268 268
Partials 46 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1. If _sign_ = 0, return ZeroDateDuration(). | ||
1. Let _duration_ be CreateDateDurationRecord(0, 0, 0, 0). | ||
1. If _largestUnit_ is ~year~, then | ||
1. Let _intermediate_ be _one_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_intermediate_
needs to be initialized to something that is on the same side of _two_
as _one_
is, so that the body of the loop runs at least once. I pick _one_
since it always satisfies that criterion.
1. Let _intermediate_ be _one_. | ||
1. Repeat, while CompareISODate(_intermediate_, _two_) != _sign_, | ||
1. Set _duration_.[[Years]] to _duration_.[[Years]] + _sign_. | ||
1. Set _intermediate_ to CalendarDateAdd(_calendar_, _one_, _duration_, ~constrain~). | ||
1. Set _duration_.[[Years]] to _duration_.[[Years]] - _sign_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop is better written as:
loop {
duration.years += sign;
let intermediate = CalendarDateAdd(calendar, one, duration, "constrain");
if CompareISODate(intermediate, two) == sign {
duration.years -= sign;
break;
}
}
but I wrote it like I did since we don't have do-while loops in ecmarkup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure we do:
1. Let _done_ be *false*.
1. Assert: The following loop will terminate.
1. Repeat, while _done_ is *false*,
1. Set _duration_.[[Years]] to _duration_.[[Years]] + _sign_.
1. Let _intermediate_ be CalendarDateAdd(_calendar_, _one_, _duration_, ~constrain~).
1. If CompareISODate(_intermediate_, _two_) is _sign_, then
1. Set _duration_.[[Years]] to _duration_.[[Years]] - _sign_.
1. Set _done_ to *true*.
Some examples:
- RegExp.prototype [ %Symbol.replace% ]
- InnerModuleLinking
- Set.prototype.difference and other Set.prototype methods («Repeat, while next is not DONE,»)
- Iterator.prototype.flatMap («Repeat, while innerAlive is true,»)
- PartitionNotationSubPattern («Repeat, while groups List is not empty,»)
1. Return ! CreateDateDurationRecord(_years_, _months_, _weeks_, _days_). | ||
1. Return an implementation-defined Date Duration Record as described above. | ||
1. Let _sign_ be -CompareISODate(_one_, _two_). | ||
1. If _sign_ = 0, return ZeroDateDuration(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double-check that my signs are correct and not negated.
- If one < two:
- CompareISODate(one, two) == -1, so sign = 1
- Inside the loop, we increment the duration by 1
- The loop condition checks CompareISODate(intermediate, two):
- The first time, intermediate is one, so the return value is -1, which does not equal sign, so the loop runs
- At some point after that, intermediate will be greater than two, at which point the return value is 1, the loop terminates, and the duration is decremented back to the value before the excess occurred
- Note: If intermediate and two are ever equal, the loop will run one last time, and the value of duration that caused the two values to be equal will become the resolved duration
- If one > two:
- CompareISODate(one, two) == 1, so sign == -1
- (all other steps should still work, except with signs flipped)
1. Let _intermediate_ be _one_. | ||
1. Repeat, while CompareISODate(_intermediate_, _two_) != _sign_, | ||
1. Set _duration_.[[Years]] to _duration_.[[Years]] + _sign_. | ||
1. Set _intermediate_ to ! CalendarDateAdd(_calendar_, _one_, _duration_, ~constrain~). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might need to refactor CalendarDateAdd in order to make it infallible. I guess it's possible that some dates really close to the boundary would resolve to intermediates that are out of bounds, but I don't want those to cause exceptions to be thrown.
@sffc Please review #2535 (expand the hidden content and search the page for "exhaust", because @ptomato wrote some great exhaustive scripts at e.g. #2535 (comment) ) and maybe also the resulting test262 coverage at tc39/test262#4004 . Simply put, guessing about the absence of a counterexample is unacceptable vs. taking a full calendar cycle such as the 1461 inclusive days from Gregorian 1970-01-01 to 1974-01-01 (inclusive) and evaluating a full cross product for each largestUnit of ['month', 'year', 'week', 'day']. |
Ok, this PR isn't editorial then. |
I think the key AO I also still want to explore whether I can make |
OK, I think I managed to get this working. See #3138 for the first incremental editorial change. |
I think I'm able to achieve my goal using the framework in tc39/proposal-intl-era-monthcode#69 and #3138. So, I will close this PR. I may come back with more smaller editorial PRs as necessary. |
This changes CalendarDateUntil so that it is based purely on CalendarDateAdd instead of using ISO-specific AOs like ISODateSurpasses and RegulateISODate. It means that the same CalendarDateUntil can be used for both ISO and non-ISO calendars.
Please double- and triple-check my work. I haven't come up with a counter-example where behavior differs, but that doesn't mean there isn't one that I didn't think of. I have not implemented this in code.