Implement date arithmetic according to Temporal specification #7012
Conversation
sffc
changed the title
|
I found some cases in the Hebrew calendar that I need to fix by fixing tc39/proposal-intl-era-monthcode#69. (This is why we have spec: so we can decide on the right thing upstream) |
|
With Robert's review and my partial review I'm comfortable landing this so you can start optimizing, I'll let you know if there are post merge review issues. And if there are test failures in V8. |
| } | ||
|
|
||
| impl<C: CalendarArithmetic> ArithmeticDate<C> { | ||
| /// Implements BalanceNonISODate |
There was a problem hiding this comment.
nit: spec link, and an in-words explanation. Balance is Temporal-internal jargon, we should explain what this does in non-Temporal words too
There was a problem hiding this comment.
I can't link to the spec because tc39/proposal-intl-era-monthcode#69 still isn't merged, but I can explain what it does
| // 1. Let _parts_ be CalendarISOToDate(_calendar_, _fromIsoDate_). | ||
| // 1. Let _y0_ be _parts_.[[Year]] + _years_. | ||
| let y0 = cal.year_info_from_extended(duration.add_years_to(self.year.to_extended_year())); | ||
| // 1. Let _m0_ be MonthCodeToOrdinal(_calendar_, _y0_, ! ConstrainMonthCode(_calendar_, _y0_, _parts_.[[MonthCode]], ~constrain~)). |
There was a problem hiding this comment.
nit: include an explanatory comment somewhere saying what this does ("transfer the month code to the new year" or something)
There was a problem hiding this comment.
I think for both this function and the one above, prose explanations of the various sections would be good. Matching the spec is nice for ensuring correctness, but if some behavior needs to be debugged it's good to have explanatory comments.
Prefer followup.
Are you blocking on the internal macro? |
There was a problem hiding this comment.
you've made it even more complicated. there are now three levels of macros that recursively call each other. we're not playing code golf here, saving 100 lines of code but adding 5 minutes of parsing for the next person that has to touch this code is not the goal.
if you want to unblock this PR just make the minimal changes to the match statements in add and until. we can discuss the benefits of this macro spaghetti in a different PR
|
The new macro saves lines of code, sure, but this isn't about just saving lines of code. It is about making the code less prone to typographical errors. I saw the add/until delegation code as already being on the precipice of being unmaintainable in this way. My addition of the new error cases and parameters pushed it over the edge. I therefore include in this same pull request a change that reduces the chance of coding errors by producing each piece of code exactly one: the match has its own macro, the pattern has its own macro, and the expression has its own macro. Reasonable people can disagree on which form is more readable, but this should not block a pull request. |
|
I'm going to start working on other PRs, which will be based on top of this one. If someone wants to push a change to the internal private macro and then approve the PR, okay, but I don't intend to spend more time on resolving internal readability/style concerns that reasonable people can disagree on. |
There was a problem hiding this comment.
Reasonable people can disagree on which form is more readable, but this should not block a pull request.
If you don't want this to block your pull request, pull it out into another pull request. This is a big change and it significantly impacts the maintainability of that module for anyone but you. It's the same situation as the macro overload in the datetime crate.
I'm happy to discuss this, and could be convinced of the benefits of your approach, but it's a completely different discussion from this PR, and I don't appreciate you trying to force it through like this.
I've pushed a commit that reduces the diff in any_calendar.rs to the minimum and we can continue the macro discussion in a different PR.
There was a problem hiding this comment.
I'm in favor of having the macro refactor being an internal followup.
As I previously discussed with @sffc I think it is fine for PRs to tend towards removing separable internal code style changes. Extracting a refactor out of a PR and applying it later is easier than undoing a refactor later when people are working on the same code.
| let date = Date::try_new_hijri_with_calendar(-300, 1, 1, cal); | ||
| check_every_250_days(date.unwrap()); | ||
| // This test is slow since it is doing astronomical calculations, so check only 100 dates | ||
| check_every_250_days(date.unwrap(), 100); |
There was a problem hiding this comment.
issue: this reduces test coverage for continuity because the try_from_fields check you're adding to this test is slow.
the old test caught the continuity breakage in #7065, but this version does not
There was a problem hiding this comment.
Ok. I assume this doesn't need a follow-up since #7065 is doing the extended range testing.
There was a problem hiding this comment.
#7075 removes the test for simulated, but your change also removes coverage for all other tests that set iters to values less than 2000, and I don't know if that hides potential issues. Please restore.
3 participants
#1174
Fixes #3147
This implementation aligns line-for-line with the spec. I intend to go back and add fast paths (like adding multiple years or months at once), but let's check in something that we know to be spec-compliant first.