-
Notifications
You must be signed in to change notification settings - Fork 154
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
Where should month without year or monthCode be rejected? #2664
Comments
I personally can live with A or B, which are both easy to explain but prioritize different aspects of behavior. But my preference is B, because A encourages either
|
@sffc do you want to weigh in here too? My take:
A consequence of my opinions above is that users of a custom calendar can never do what the ISO calendar does, which is allow creating a PlainMonthDay without including the year in the input. IMO this is OK, because it seems like a tiny set (maybe the null set?) of custom calendars that will be able to deterministically map all yearless month/day pairs to a specific ISO date. One process note: whatever we do here, it's not a new normative change that we need to take to plenary. Rather, I see it as a bug fix to #2500 which was previously approved. |
I'm a little iffy about 3 and 4 having different behavior (built-in calendar ID string vs calendar object) but I can see the argument both ways. So my preference would be to say that in |
I'm fine with Shane's recommendation above. |
I agree we should fix the consistency bug that slipped into #2500. I also agree that custom calendars don't need to have the capability to accept yearless |
@gibson042, would you be OK to accept @sffc's proposal above? (excerpting below)
I'm agnostic about If you're OK with this plan then I think we can unblock all the other PRs on this topic. If not, then let's discuss! |
I give reasons in the top description for why I believe that 4.i and 4.iii should be accepted (specifically because of 5.i, which is lower-level than 2 and must be valid for 2 to make sense). Discussion it is! |
Meeting 2023-09-14:
This fix goes in https://tc39.es/proposal-temporal/#sec-temporal-totemporalmonthday:
Current:
Desired:
|
There's a problem with the above plan—some calendars accept EDIT: However, it looks like even 3.iii isn't currently being rejected. Here's one way to fix that (an alternative is early errors, but those are easier to overlook in my opinion): diffdiff spec/abstractops.html
--- a/spec/abstractops.html
+++ b/spec/abstractops.html
@@@ -1375,8 -1375,8 +1375,9 @@@ ParseISODateTime (
1. For each |Annotation| Parse Node _annotation_ contained within _parseResult_, do
1. Let _key_ be the source text matched by the |AnnotationKey| Parse Node contained within _annotation_.
1. Let _value_ be the source text matched by the |AnnotationValue| Parse Node contained within _annotation_.
- 1. If CodePointsToString(_key_) is *"u-ca"*, and _foundCalendar_ is *undefined*, then
- 1. Set _foundCalendar_ to CodePointsToString(_value_).
+ 1. If CodePointsToString(_key_) is *"u-ca"*, then
+ 1. If _goal_ is |TemporalMonthDayString|, throw a *RangeError* exception.
+ 1. If _foundCalendar_ is *undefined*, set _foundCalendar_ to CodePointsToString(_value_).
1. If _foundCalendar_ is not *undefined* and the ASCII-lowercase of _foundCalendar_ is not *"iso8601"*, throw a *RangeError* exception.
1. If _parseResult_ is not a Parse Node, throw a *RangeError* exception.
1. Let each of _year_, _month_, _day_, _hour_, _minute_, _second_, and _fSeconds_ be the source text matched by the respective |DateYear|, |DateMonth|, |DateDay|, |TimeHour|, |TimeMinute|, |TimeSecond|, and |TimeFraction| Parse Node contained within _parseResult_, or an empty sequence of code points if not present.
diff polyfill/lib/ecmascript.mjs
--- a/polyfill/lib/ecmascript.mjs
+++ b/polyfill/lib/ecmascript.mjs
@@@ -557,8 -557,8 +557,8 @@@ export function ParseTemporalMonthDayString(isoString) {
month = ToIntegerOrInfinity(match[1]);
day = ToIntegerOrInfinity(match[2]);
calendar = processAnnotations(match[3]);
- if (calendar !== undefined && calendar !== 'iso8601') {
- throw new RangeError('MM-DD format is only valid with iso8601 calendar');
+ if (calendar !== undefined) {
+ throw new RangeError('MM-DD format is not valid with a calendar annotation');
}
} else {
let z; |
I'll offer another solution. I edited the OP and added some more lines (not changing the numeric identifiers of existing ones). They use the calendar "gregory". We could make all of the built-in human calendars require the month code or year disambiguation, but let "iso8601" continue to accept numeric month and year. This would somewhat reduce the potential of data-driven exceptions because programmers introducing a calendar system likely do so because they want something other than iso8601. We can inform those programmers quickly by making all of the built-in CLDR calendars require the same shape of the fields. This would mean the following results:
|
I think this argument sounds reasonable, esp. given the problem that Richard found that made the previous solution won't work. @sffc - could you fix your numbering above? There is no 3.v, 3.vi, etc. |
There is; I added them as noted above. |
Oh, whoops my page didn't refresh. Now I see. Your proposal sounds good to me.
Does that mean it's up to the calendar to decide whether to throw or not? If so, then I support. |
Good news: that is the current state of #2663. |
Built-in non-ISO calendars require either monthCode/day, or month/day plus some form of year specification. This adds test coverage for each of the categories listed in tc39/proposal-temporal#2664, of which some must currently reside in the test/intl402/ folders.
Built-in non-ISO calendars require either monthCode/day, or month/day plus some form of year specification. This adds test coverage for each of the categories listed in tc39/proposal-temporal#2664, of which some must currently reside in the test/intl402/ folders.
Yes that's what it means. Calendar decides when to throw. ECMAScript establishes a convention that ISO-8601 doesn't throw on missing monthCode, but all human calendars do. |
Built-in non-ISO calendars require either monthCode/day, or month/day plus some form of year specification. This adds test coverage for each of the categories listed in tc39/proposal-temporal#2664, of which some must currently reside in the test/intl402/ folders.
Built-in non-ISO calendars require either monthCode/day, or month/day plus some form of year specification. This adds test coverage for each of the categories listed in tc39/proposal-temporal#2664, of which some must currently reside in the test/intl402/ folders.
Built-in non-ISO calendars require either monthCode/day, or month/day plus some form of year specification. This adds test coverage for each of the categories listed in tc39/proposal-temporal#2664, of which some must currently reside in the test/intl402/ folders.
(derived from #2663 (comment) )
(EDITED by @sffc on 2023-09-15)
Temporal.PlainMonthDay.from({ monthCode: "M08", day: 1, calendar: "iso8601" })
Temporal.PlainMonthDay.from({ monthCode: "M08", day: 1, calendar: "hebrew" })
Temporal.PlainMonthDay.from({ monthCode: "M08", day: 1, calendar: "gregory" })
Temporal.PlainMonthDay.from({ monthCode: "M08", day: 1 })
Temporal.PlainMonthDay.from({ month: 8, day: 1 })
Temporal.PlainMonthDay.from("08-01")
Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: "iso8601" })
Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: "hebrew" })
(must reject for ambiguity)Temporal.PlainMonthDay.from("08-01[u-ca=iso8601]")
Temporal.PlainMonthDay.from("08-01[u-ca=hebrew]")
(must reject for ambiguity)Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: "gregory" })
Temporal.PlainMonthDay.from("08-01[u-ca=gregory]")
Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: Temporal.Calendar.from("iso8601") })
Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: Temporal.Calendar.from("hebrew") })
(must reject for ambiguity)Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: customCalendar })
Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: Temporal.Calendar.from("gregory") })
monthDayFromFields
with a specific valid calendar instance receiverTemporal.Calendar.from("iso8601").monthDayFromFields({ month: 8, day: 1 })
Temporal.Calendar.from("hebrew").monthDayFromFields({ month: 8, day: 1 })
(must reject for ambiguity)Temporal.Calendar.from("gregory").monthDayFromFields({ month: 8, day: 1 })
I believe the only potentially variant points are 3.i, 3.iii, 4.i, 4.iii, and 5.i. The current spec as of #2500 is to reject 3.iii but accept the rest, and the previous state was to reject all but 4.iii. I feel strongly that 5.i should be accepted because of 2, and therefore that 4.i and 4.iii should be accepted as well. If that has agreement, then the only remaining questions are 3.i and 3.iii, where 3.iii has been rejected for a long time and is not in contention AFAIK. So I'll start a list of two options and anticipate potential growth:
A: Reject 3.i.
calendar: string
always requires month to be qualified by year and/or monthCode (inconsistent betweencalendar: "iso8601"
vs.calendar: Temporal.Calendar.from("iso8601")
but otherwise similar to pre-#2500 and in particular always rejects{ month, day, calendar: str }
but delegates{ month, day, calendar: obj }
to the calendar).B: Accept 3.i. ISO 8601 calendar always resolves month to monthCode (current state; consistent between string vs. object ISO 8601 calendar but rejection of
{ month, day, calendar: str }
is dependent upon the value of calendar rather than the total input shape).The text was updated successfully, but these errors were encountered: