Editorial: Align non-ISO calendar validation with ISO behaviour #2940
Conversation
gibson042
changed the title
gibson042
changed the title
gibson042
force-pushed
the
gh-2864-universal-year-and-month-consistency
branch
from
cd68da4
to
fa3c85e
Compare
gibson042
force-pushed
the
gh-2864-universal-year-and-month-consistency
branch
2 times, most recently
from
0b67f86
to
ff30462
Compare
gibson042
changed the title
gibson042
force-pushed
the
gh-2864-universal-year-and-month-consistency
branch
from
ff30462
to
98a8777
Compare
| const checkField = (name, value) => { | ||
| const checkField = (name, value, aliases) => { | ||
| const currentValue = calendarDate[name]; | ||
| if (currentValue != null && currentValue != value) { | ||
| if ( | ||
| currentValue != null && | ||
| currentValue != value && | ||
| !ES.Call(ArrayPrototypeIncludes, aliases || [], [currentValue]) | ||
| ) { |
There was a problem hiding this comment.
I think this change (and the corresponding adaptation to return eraAliases below) could use some more attention in tests. I believe it's correct and necessary, but there aren't any tests that fail if I revert it. I propose that we still go ahead and merge this but if you have an idea for a test case, please add it to test262 in a follow up.
There was a problem hiding this comment.
Hmm, let's see... without this use of aliases, a call like Temporal.PlainDate.from({ calendar: "gregory", era: "ce", eraYear: 2024, year: 2000, month: 1, day: 1 }) (having year along with era and eraYear) fails with RangeError "Input era ce doesn't match calculated value gregory". So there's your test case input, although I'm not sure where to put it in test262 so can I ask you to do that and I'll review?
Also, I feel like the polyfill has terrible choices for the primary names of Gregorian calendar eras—the above should probably read like "…doesn't match calculated value ce" or "…doesn't match calculated value ad", and the message for Temporal.PlainDate.from({ calendar: "gregory", era: "ce", eraYear: 0, year: 0, month: 1, day: 1 }) should probably be "Input era ce doesn't match calculated value bce" or "Input era ce doesn't match calculated value bc" rather than "Input era ce doesn't match calculated value gregory-inverse" because what the heck is "gregory-inverse" to the median developer?
There was a problem hiding this comment.
Thanks for the test case. This is done in tc39/test262#4232. I checked that it fails when you revert the aliases fix.
Re. the era names, our hands are tied, as I'm trying to follow the current state of the Intl Era and Month Codes proposal. Justin has a ticket open for improving the canonical names. I guess in the polyfill we could prefer printing an alias in the error messages if there is one.
There was a problem hiding this comment.
Thanks; test262 PR approved.
And I just now left a comment on the era/month code issue; we're right to keep the polyfill aligned with it but hopefully that part will improve.
There was a problem hiding this comment.
Maybe we could show both the month code proposal names and the more recognizable alias(es) in the error message?
There was a problem hiding this comment.
Tentatively, that's what we're doing now since #2944. If the era/monthcode proposal changes, we can change it to do something else.
There was a coverage gap for converting calendar fields to a date when the provided era was an alias. See tc39/proposal-temporal#2940 (comment)
There was a coverage gap for converting calendar fields to a date when the provided era was an alias. See tc39/proposal-temporal#2940 (comment)
There was a coverage gap for converting calendar fields to a date when the provided era was an alias. See tc39/proposal-temporal#2940 (comment)
Fixes #2863
Fixes #2864
Fixes #2866
Updated recommendations for non-ISO calendars:
test262 PR: tc39/test262#4231
Help wanted with further testing and thorough verification of correctness and coverage.