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 2 commits
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
55 changes: 53 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_: a positive 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,16 @@ 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 if _day_ &lt; 1, then
1. If _overflow_ is ~reject~, throw a *RangeError* exception.
1. Let _regulatedDay_ be 1.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we can get here - Calendar Fields Records can only have a positive integer or UNSET in the [[Day]] field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, good catch

1. Else,
1. Let _regulatedDay_ be _day_.
1. Return ? CalendarIntegersToISO(_calendar_, _arithmeticYear_, _ordinalMonth_, _regulatedDay_).
</emu-alg>
</emu-clause>

Expand Down