-
Notifications
You must be signed in to change notification settings - Fork 8
Use more specific calendar-dependent operations in Until, Add, and ToISO #69
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
base: main
Are you sure you want to change the base?
Conversation
a72de40
to
12195b9
Compare
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.
Partial review up til NonISODateSurpasses
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.
Very minor points overall. Looks good!
Implementing this efficiently may be tricky but that's fine; at least now we have more concrete guidance for behavior
A bit more commentary on my choices of AOs:
|
To put Add and Until's algorithm in words:
The key difference with non-ISO is step 2. ISO doesn't need to constrain to a valid year/month because all months are valid in all years. This also feeds into #56. Is it more consistent or less consistent for non-ISO to reject when constraining year/month? I took the position that non-ISO is better off rejecting on step 4 only. I think it's a valid position that it should reject on both steps 2 and 4. However, maybe there should be a third option in the enum to allow for either behavior. |
This PR was included in the Stage 2.7 presentation. This PR was approved as part of Stage 2.7 with the following understanding: "changes the spec text to transform some parts of the spec from prose to algorithmic spec steps" |
I believe I have responded to all of the threads. PTAL. Thank you @Manishearth @gibson042 @ryzokuken! |
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.
Minor suggestion: the adjective for "arithmetic" can be "arithmetical" but could also be "arithmetic", e.g. "arithmetic mean".
Brevity is important in spec text, so I'd suggest globally replacing "arithmetical" wtih just "arithmetic".
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.
Overall looks good. A few minor suggestions only.
I prefer using the explicitly-adjective "arithmetical", but I was actually thinking of using a term like "monotonic" to emphasize how they actually behave, in another PR. But since this spec currently uses "arithmetic", I can keep using that term to reduce the diff, if other people feel the difference warrants fixing. |
I'll integrate @justingrant's suggestions, but I'm waiting for the other re-reviews to come in first so that I can update the PR at one time. |
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.
The month code stuff is all great. In order to facilitate merging this PR so that we don't have to do a follow up, I've opened a Temporal PR tc39/proposal-temporal#3144 to include it in Temporal.
I'm less convinced about the changes to NonISODateAdd, NonISODateUntil, and NonISOCalendarDateToISO. I preferred them the way they were in the prose form, because I think that's sufficiently well-defined to allow for unambiguous implementation while also not re-specifying stuff that is already in some external library such as ICU. This form seems more bug-prone to me, since realistically what are implementations going to do other than use ICU? By rewriting ICU in spec text here, that is never going to be validated by someone directly writing code that follows the spec, we're just inviting busywork on ourselves when they inevitably don't match in some edge case.
Ultimately I'd leave it up to the 402 editors whether they want that, but my recommendation would be no.
(I'd still like to use ConstrainMonthCodeToArithmeticalMonth in NonISODateAdd though.)
What I do feel more strongly about: I do not see it as a desirable goal to refactor NonISODateAdd and NonISODateUntil back into CalendarDateAdd and CalendarDateUntil. That loses the separation between machine calendar and human calendar. I would prefer that the ISO operations remain completely separate. I get that it is aesthetically more pleasing to unify them! But I think that is the same type of mistake I made with user-defined calendars, trying to express all of Temporal's operations in terms of calls to dateAdd
and dateUntil
, inviting lots of churn and additional bugs as various well-meaning people figured out ever-slightly-more-optimized ways to express it. Only in this case the abstraction is one level lower. And most likely, implementations are not going to write their code in terms of these low-level operations, they are going to use ICU.
}, | ||
{ | ||
"location": "https://tc39.es/ecma262/", | ||
"entries": [ | ||
{ | ||
"type": "op", | ||
"aoid": "ℝ", | ||
"id": "ℝ" | ||
} | ||
] |
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.
Nothing to do with the substance of the PR, but I'm surprised this doesn't get linked automatically with --load-biblio @tc39/ecma262-biblio
!
spec.emu
Outdated
1. Repeat, while _number_ ≤ 12, | ||
1. Let _currentMonthCode_ be CreateMonthCode(_number_, _isLeap_). | ||
1. If YearContainsMonthCode(_calendar_, _year_, _currentMonthCode_), then | ||
1. Set _monthsBefore_ to _monthsBefore_ + 1. | ||
1. If _currentMonthCode_ is _resolvedMonthCode_, then | ||
1. Return _monthsBefore_. | ||
1. If _isLeap_ is *false*, then | ||
1. Set _isLeap_ to *true*. | ||
1. Else, | ||
1. Set _isLeap_ to *false*. | ||
1. Set _number_ to _number_ + 1. | ||
1. Assert: The above loop should have returned before terminating. |
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.
You might be able to simplify this somehow since month codes are lexicographically ordered, but it's probably not worth it unless you're really enthusiastic about it.
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 think the way I do the loop is fine, unless you have a specific suggestion to improve it.
spec.emu
Outdated
</emu-alg> | ||
</emu-clause> | ||
|
||
<emu-clause id="sec-temporal-calendardaysinmonth" type="implementation-defined abstract operation"> |
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.
Minor point, I would suggest deleting the one-line calendar-dependent operations CalendarDaysInMonth and CalendarMonthsInYear. We used to have those in Temporal and got rid of them because they take up space without really adding anything meaningful. Instead, I'd just put something in NonISODateAdd etc. like "Let monthsInYear be the calendar-dependent number of months in year."
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.
@gibson042 suggested smaller AOs earlier in #69 (comment). I don't have a strong opinion except that it's kind-of nice to have a "googlable" place to plug things in.
Nice!
"I"* am an implementer and also an ICU contributor. I have a bug assigned to me to implement calendar arithmetic in ICU[4X]. I don't know what to do: there is no spec to follow. I can try to replicate existing behavior, but I don't always know exactly what it does, and I can't authoritatively test that I did it correctly. The CLDR standard doesn't specify this because it deals only with formatting. I can read the prose of Temporal / ECMA-402, but what I would rather do is read a spec algorithm and then test my code against the spec's test suite. So, yes, this spec logic is likely to be implemented in ICU more than in the engine or in Temporal_rs, but the logic is more squarely in Temporal's domain than in CLDR's domain. * "I" is in quotes because although I am in this situation, it is a situation that isn't necessarily specific to me: it could be for any implementer.
OK. I won't do that then. (This PR does not touch CalendarDateAdd and CalendarDateUntil.) I realized that writing them in terms of the lower-level operations I added here like I may desire to write an editorial PR to align Would you like me to (or would you like to) pull |
I agree that lack of specification is a valid concern for ICU4X, but I personally don't think it belongs in ECMA-402 — being an authority on calendrical calculations seems like scope creep, just like it'd be scope creep to specify that the translation of Wednesday formatted in German is Mittwoch. Anyway, ultimately scope decisions are up to the editors. |
I ... don't think these are remotely comparable cases. Formatting is a case where there are clear locale-dependent preferences. The answer to "what happens when you add 1, 2, or 3 months to Jan 31", on the other hand, is not the type of nuanced choice that most calendars have their own opinion on. Or "when you add one month and one day to a date, do you add the month first or the day first?". These are cases where someone needs to make a choice as to what "add" really means. We cannot defer to calendar norms for an answer for this (these norms do not exist), a choice needs to be made here. Given that there are lots of nuanced edge cases here, people approaching this with a specific need for Furthermore, in practice, if this is left undefined, what's going to happen is each implementor will either use an existing API that has some behavior, or spend their own time generalizing the ISO arithmetic algorithm's behavior. I don't think that's a good place for the spec to be. Now, there is some calendar-specific stuff involved here, specifically #70 . I'm comfortable with that being something we declare to be out of scope: the current PR has a table entry on the calendar for this, which I think is quite nice, but I would understand saying that this is a case where there are nuanced preferences and having it truly be implementation-defined. |
I guess fundamentally I consider this type of arithmetic to be in a different bucket of calculations than "calendrical calculations". Even the primary resource on the subject of calendrical calculations, the book of the same name, does not specify this type of thing. It exhaustively specifies the structure of various calendars, the ways to calculate that structure and perform conversions, but it does not specify any sort of arithmetic operations for moving between dates. And it's easy to see why: the nuance involved in algorithms for that is a matter of algorithmic choice (what you are trying to achieve), not a matter of calendar-specific behavior. It's a miscategorization to lump this alongside the regular calendrical calculations. |
CLDR is the standard that defines "the translation of Wednesday formatted in German is Mittwoch", and ECMA-402 correctly references it. There are standards or other entities that define calendrical calculations*, like GB/T 33661–2017, and Intl Era Month Code correctly defers to them. But, we're talking about calendar arithmetic. The closest we have to a standard for calendar arithmetic is RFC 7529, which is where we got Month Codes from, but it stops short of defining a general algorithm. Temporal is defining the concept of a "ordinal month" and "arithmetic year", and re-defining and tying together concepts such as "era year" and "month code". It needs to either point somewhere for calendar arithmetic, or just define it itself. Temporal already defines plenty of algorithms. I know of other, non-JS datetime APIs that defer to Temporal to answer other algorithmic questions, like how to deal with overflow, durations, balancing, conflicting fields, etc. This is all to say that I think ECMA-402 is an appropriate home for a calendar arithmetic standard. * When I say "calendrical calculations", I am using it in the sense that Reingold uses it: it's a method used to represent epoch days. "Fixed to calendar date" and "calendar date to fixed" are in scope, but not "add 1 year and what do you get". |
I plan to rebase this PR on top of #73. I've addressed almost all of the outstanding comments on this PR, and I will highlight any other changes between the current state of the PR and the updated version. |
Capitalize "Boolean" RegulateNonISOYearMonth => ConstrainMonthCodeToArithmeticalMonth Change unqualified _month_ to _arithmeticalMonth_ Label a few things as "positive integer" fromIsoDate, toIsoDate
f157b7a
to
8138fc3
Compare
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 think many uses of integer
in this specification text should actually be positive integer
.
Co-authored-by: Richard Gibson <[email protected]>
I quite like CalendarDateArithmeticalToISO so I pulled it out into another PR. #74 |
Fixes #55. More steps toward #57.
This PR is based on #66, and it also depends on #68 and tc39/proposal-temporal#3138.
I rewrote NonISODateAdd, NonISODateUntil, and NonISOCalendarDateToISO to more precisely specify their algorithms. They are still ultimately calendar-dependent, but the calendar-dependent parts are boiled down to more bite-sized abstract operations:
CalendarDateArithmeticalToISO
CalendarDaysInMonth
CalendarMonthsInYear
YearContainsMonthCode