Skip to content

Editorial: Refactor the last bit of prose in NonISOCalendarDateToISO to AOs #74

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 11, 2025
Merged

Editorial: Refactor the last bit of prose in NonISOCalendarDateToISO to AOs #74

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad82406
Pull out CalendarIntegersToISO, CalendarDaysInMonth
sffc Aug 15, 2025
61325b1
Day should be positive
sffc Aug 22, 2025
7611c40
Review feedback
sffc Sep 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions spec.emu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,26 @@ contributors: Google, Ecma International
</emu-alg>
</emu-clause>

<emu-clause id="sec-temporal-calendardaysinmonth" type="implementation-defined abstract operation">
<h1>
CalendarDaysInMonth (
_calendar_: a calendar type that is not *"iso8601"*,
_arithmeticYear_: an integer,
_ordinalMonth_: a positive integer,
): an integer
</h1>
<dl class="header">
<dt>description</dt>
<dd>
It returns the number of days in the _calendar_-specific _arithmeticYear_ and _ordinalMonth_.
</dd>
</dl>
<p>It performs the following steps when called:</p>
<emu-alg>
1. Return the number of days in the month of _calendar_ corresponding to _arithmeticYear_ and _ordinalMonth_.
</emu-alg>
</emu-clause>

<emu-clause id="sec-temporal-isvaliderayearforcalendar" type="abstract operation">
<h1>
IsValidEraYearForCalendar (
Expand Down Expand Up @@ -844,6 +864,29 @@ contributors: Google, Ecma International
</emu-alg>
</emu-clause>

<emu-clause id="sec-temporal-calendarintegerstoiso" type="implementation-defined abstract operation">
<h1>
CalendarIntegersToISO (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this operation adds much when pulled out into its own AO, and everywhere else we use year/month code/day to refer unambiguously to a date, not the ordinal month. (Same thing for CalendarDaysInMonth.) I don't see this operation on the ICU4X flowchart either? Should it conceptually be part of ICU4X and not Temporal?
I guess you probably have some purpose in mind for it.

If you decide to keep it I'd probably look for a different name. I find the term "calendar integers" confusing, how about NonISOCalendarDateWithOrdinalMonthToISO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This AO makes more sense in the next PR but I can hold it for then if you prefer

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is valuable even in the context of the current PR because it makes the calendar-specific algorithm be isolated to a small AO.

_calendar_: a calendar type that is not *"iso8601"*,
_arithmeticYear_: an integer,
_ordinalMonth_: a positive integer,
_day_: an integer,
): either a normal completion containing an ISO Date Record or a throw completion
</h1>
<dl class="header">
<dt>description</dt>
<dd>
It returns an ISO Date Record that corresponds with the given _calendar_-specific _arithmeticYear_, _ordinalMonth_, and _day_.
</dd>
</dl>
<p>It performs the following steps when called:</p>
<emu-alg>
1. If _arithmeticYear_, _ordinalMonth_, and _day_ do not form a valid date in _calendar_, throw a RangeError exception.
1. Let _isoDate_ be an ISO Date Record such that CalendarISOToDate(_calendar_, _isoDate_) returns a Calendar Date Record whose [[Year]], [[Month]], and [[Day]] field values respectively equal _arithmeticYear_, _ordinalMonth_, and _day_.
1. Return _isoDate_.
</emu-alg>
</emu-clause>

<emu-clause id="sup-temporal-calendar-date-records">
<h1>Calendar Date Records</h1>
<p>
Expand Down Expand Up @@ -1087,8 +1130,13 @@ contributors: Google, Ecma International
1. Let _ordinalMonth_ be _fields_.[[Month]].
1. Let _day_ be _fields_.[[Day]].
1. Assert: _day_ is not ~unset~.
1. If the date described by _calendar_, _arithmeticYear_, _ordinalMonth_, and _day_ does not exist, set _day_ to the closest day in the same month. If there are two equally-close dates in the same month, pick the later one.
1. Return an implementation-defined ISO Date Record that corresponds to the date described by _calendar_, _arithmeticYear_, _ordinalMonth_, and _day_.
1. Let _daysInMonth_ be CalendarDaysInMonth(_calendar_, _arithmeticYear_, _ordinalMonth_).
1. If _daysInMonth_ &lt; _day_, then
1. If _overflow_ is ~reject~, throw a *RangeError* exception.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that this RangeError isn't in the prose being edited, but I think it was wrongfully removed in #73. Previously the spec said

        1. If _fields_ describes an existing date in _calendar_, return an implementation-defined ISO Date Record that corresponds to the date described by _fields_.
        1. If _overflow_ is ~reject~, throw a *RangeError* exception.

So it would have thrown on an invalid day.

1. Let _regulatedDay_ be _daysInMonth_.
1. Else,
1. Let _regulatedDay_ be _day_.
1. Return ? CalendarIntegersToISO(_calendar_, _arithmeticYear_, _ordinalMonth_, _regulatedDay_).
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this spec works by interpreting day as an ordinal day. I don't think we use any other type of day. I think this AO reads as though @ptomato had thought that day should be the day number as opposed to an ordinal day. I'm open to using language such as ordinalDay to make my interpretation more clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the definition of [[Day]]:

          <tr>
            <td>[[Day]]</td>
            <td>a positive integer</td>
            <td>
              The 1-based ordinal position of the date's day within its month.
            </td>
          </tr>

It already says it is the ordinal day, not the day number.

Should we still be more clear?

The edge case, of course, are dates such as October 10, 1867 that don't exist in certain regions like Alaska due to Gregorian switchover. If you write code such as

Temporal.PlainDate.from({ calendar: "gregory-usak", year: 1867, month: 10, day: 10 })

should it:

  1. Constrain down to October 5, 1867
  2. Constrain up to October 18, 1867
  3. Interpret the day as an ordinal and return October 22, 1867 (the 10th day in October that year)

Option 3 is much easier implementation-wise than anything else.

This is also more of an academic question at this point because we don't have julian-gregorian support.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see your point now about this looking like it refers to non-ordinal days. I intended it to refer to days that ran off the end of a month, like February 30. I got the language from NonISOCalendarDateToISO and probably "If there are two equally-close dates in that month, pick the later one" should be removed from there. I would guess that was written before we defined day as ordinal day (which happened in a normative change in January 2023)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, in CalendarYearMonthFromFields:

  1. Let firstDayIndex be the 1-based index of the first day of the month described by fields (i.e., 1 unless the month's first day is skipped by this calendar.)

</emu-alg>
</emu-clause>

Expand Down