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

Jump to bottom

PrepareCalendarFields: Consider merging "calendarFieldNames" and "nonCalendarFieldNames" parameters into a single parameter #3001

Closed

anba opened this issue Oct 7, 2024 · 2 comments

Assignees

Labels

editorial spec-text

Contributor

anba commented Oct 7, 2024

Without user-defined calendars it's probably no longer necessary to have separate calendarFieldNames and nonCalendarFieldNames parameters which later get merged anyway. And passing additional field names to CalendarExtraFields shouldn't matter, because this operation is now completely implementation-defined.

Also maybe consider sorting the field names when calling PrepareCalendarFields into the natural order instead sorting alphabetically. IMO this improves readability.

Currently in Temporal.PlainDateTime.prototype.with:

« day, month, month-code, year », « hour, microsecond, millisecond, minute, nanosecond, second »

When using a single list and sorting the field names per Table 20:

« year, month, month-code, day, hour, minute, second, millisecond, microsecond, nanosecond »

At least to me, the latter is easier to read.

And maybe consider moving PrepareCalendarFields into ch. "12 Calendars" instead of "13 Abstract Operations"?

The text was updated successfully, but these errors were encountered:

ptomato self-assigned this

ptomato added spec-text editorial labels

Collaborator

ptomato commented Oct 8, 2024

I'm not convinced about merging calendarFieldNames and nonCalendarFieldNames. I don't want to imply that CalendarExtraFields should be able to have its behaviour depend on nonCalendarFieldNames, even if it is now only for builtin calendars.

The other stuff, sounds good.

ptomato added a commit that referenced this issue


          Editorial: Move PrepareCalendarFields into calendar section

c251c03

See: #3001

ptomato added a commit that referenced this issue


          Editorial: Give field names lists to PrepareCalendarFields in table o…

e4919fe

…rder

It's easier to inspect at a glance which fields are included when they are
given in order. This order is not observable because the list is sorted
by lexicographical order anyway before being used to access properties.

See: #3001

ptomato mentioned this issue

Editorial: More suggestions from Anba #3004

Merged

ptomato added a commit that referenced this issue


          Editorial: Move PrepareCalendarFields into calendar section

57d4550

See: #3001

ptomato added a commit that referenced this issue


          Editorial: Give field names lists to PrepareCalendarFields in table o…

c50ad74

…rder

It's easier to inspect at a glance which fields are included when they are
given in order. This order is not observable because the list is sorted
by lexicographical order anyway before being used to access properties.

See: #3001

Ms2ger pushed a commit that referenced this issue


          Editorial: Move PrepareCalendarFields into calendar section

9b595c7

See: #3001

Ms2ger pushed a commit that referenced this issue


          Editorial: Give field names lists to PrepareCalendarFields in table o…

a6e5c20

…rder

It's easier to inspect at a glance which fields are included when they are
given in order. This order is not observable because the list is sorted
by lexicographical order anyway before being used to access properties.

See: #3001

Ms2ger pushed a commit that referenced this issue


          Editorial: Move PrepareCalendarFields into calendar section

1ef906e

See: #3001

Ms2ger pushed a commit that referenced this issue


          Editorial: Give field names lists to PrepareCalendarFields in table o…

e049468

…rder

It's easier to inspect at a glance which fields are included when they are
given in order. This order is not observable because the list is sorted
by lexicographical order anyway before being used to access properties.

See: #3001

Ms2ger pushed a commit that referenced this issue


          Editorial: Move PrepareCalendarFields into calendar section

0fab864

See: #3001

Ms2ger pushed a commit that referenced this issue


          Editorial: Give field names lists to PrepareCalendarFields in table o…

e6660f2

…rder

It's easier to inspect at a glance which fields are included when they are
given in order. This order is not observable because the list is sorted
by lexicographical order anyway before being used to access properties.

See: #3001

Collaborator

ptomato commented Oct 8, 2024

Closed in #3004.

ptomato closed this as completed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment