Skip to content
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

Normative: Add firstDayOfWeek and consider "-u-fw" #70

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Conversation

FrankYFTang
Copy link
Collaborator

Also change the return value of getWeekInfo()
to use string instead of integer.

@FrankYFTang
Copy link
Collaborator Author

@anba

Copy link
Contributor

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Question: Do we need to do anything else to clarify that the value of the extension keyword can change the outcome of WeekInfoOfLocale, or is that already covered by the hand-wavy

Return a record whose fields are defined by <emu-xref href="#table-locale-weekinfo-record"></emu-xref>, with values based on _locale_.

@anba
Copy link
Contributor

anba commented Jun 5, 2023

Question: Do we need to do anything else to clarify that the value of the extension keyword can change the outcome of WeekInfoOfLocale, [...]

I think it should be clarified in more detail → #30.

@anba
Copy link
Contributor

anba commented Jun 5, 2023

This is also changing [[FirstDay]] and [[Weekend]] from numeric values to string values. I thought we intentionally used numeric values to:

  1. Not tie weekday names to a specific calendar.
  2. Match Temporal, which also uses integers from 1-7, cf. ToISODayOfWeek and CalendarDateDayOfWeek.

So we have to decide if it's more important to keep the values returned from Intl.Locale.prototype.getWeekInfo() consistent with the Unicode extension values for u-fw or with the Temporal API.

See also #73 for another case where Unicode extension values and values returned from Intl.Locale won't match.


Edit: Also mentioned in #68 (comment).

@FrankYFTang
Copy link
Collaborator Author

I totally agree we need to discuss the issue about the return value (string vs number) more.
We may need to treat the value returned by
Intl.Locale.prototype.firstDayOfWeek
and
Intl.Locale.prototype.getWeekInfo()
differently.

@FrankYFTang
Copy link
Collaborator Author

Another ref https://www.unicode.org/reports/tr35/tr35-31/tr35-dates.html#Week_Data

<!ELEMENT firstDay EMPTY >
<!ATTLIST firstDay day (sun | mon | tue | wed | thu | fri | sat) #REQUIRED>
<!ATTLIST firstDay territories NMTOKENS #REQUIRED>

@FrankYFTang
Copy link
Collaborator Author

@FrankYFTang
Copy link
Collaborator Author

https://tc39.es/ecma262/#sec-week-day
Basically we will have three way to represent Sunday

  • Date - Sunday = 0
  • Temporal- Sunday = 7
  • Intl.Locale.firstDayOfWeek / "-u-fw-sun" in locale / {firstDayOfWeek: "sun"} => "sun"

so the question is which representation should be used for Intl.Locale.getWeekInfo().firstDay and Intl.Locale.getWeekInfo().weekend

@FrankYFTang
Copy link
Collaborator Author

TG2 agree that we should keep using 1-7 for value inside getWeekInfo() sync w/ Temporal
TG2 also suggest keep firstDay (not change to firstDayOfWeek for the return object of getWeekInfo()) since it is in number instead of string form.
Revert that part of the change in this PR after that.

@FrankYFTang FrankYFTang requested a review from sffc July 5, 2023 16:43
@FrankYFTang
Copy link
Collaborator Author

This is also changing [[FirstDay]] and [[Weekend]] from numeric values to string values.

Just to make it clear. that part of changes is already reverted and removed from this PR after our last TG2 meeting.

@FrankYFTang
Copy link
Collaborator Author

@anba I added some more changes to make it sure if we have -u-fw- we will override it.
Reviewers could you please take another look and give me feedback before TC39 in July 11?

@FrankYFTang
Copy link
Collaborator Author

hum... one issue need to be clearify by Mark in https://unicode-org.atlassian.net/browse/CLDR-16839

@FrankYFTang
Copy link
Collaborator Author

The changes to WeekInfoOfLocale may be invalid - depending on the answer on https://unicode-org.atlassian.net/browse/CLDR-16839

@FrankYFTang
Copy link
Collaborator Author

@FrankYFTang
Copy link
Collaborator Author

The PR in the current state does not reach consensus in TC39 during July 12, 2023 meeting

I put down 4 new proposals in #68 (comment) Let's discuss the option there

@FrankYFTang
Copy link
Collaborator Author

The topic has no enough time to be discussed during last TG2 but a big group of people share their opion within 10 minutes of the official end of TG2 and favor

Option C (Also mentioned in the TC39)

I. Intl.Locale.prototype.firstDayOfWeek returns: <<undefined, 1, 2, 3, 4, 5, 6, 7>>
2. {firstDayOfWeek} for new Intl.Locale() accepts: <<undefined, "mon", "tue", "wed", "thu", "fri", "sat", "sun", 0, 1, 2, 3, 4, 5, 6, 7>> Default: undefined
where 0, 7, "sun" all repesent Sunday in option reading

I will change this PR to reflect that opinion soon and discuss that in TG1

@FrankYFTang FrankYFTang force-pushed the firstDayOfWeek branch 2 times, most recently from 9e90ce2 to 4ff7bad Compare September 13, 2023 03:34
Also change the return value of getWeekInfo()
to use string instead of integer.
@FrankYFTang
Copy link
Collaborator Author

PTAL

@FrankYFTang FrankYFTang requested a review from anba September 13, 2023 16:44
@FrankYFTang
Copy link
Collaborator Author

This PR is ready to review. Be aware it handle the "fw" keyword in the locale and return firstDayOfWeek in the set of <1 .. 7>
The impact of ca = "iso8601" is NOT handled in this PR (For issue #30 ) For the ca issue, it is waiting for the resolution of https://unicode-org.atlassian.net/browse/CLDR-16866 which should come out soon. So that part will be a different normative PR after the merge of this one.

@FrankYFTang FrankYFTang changed the title Add firstDayOfWeek and consider "-u-fw" Normative: Add firstDayOfWeek and consider "-u-fw" Sep 13, 2023
@FrankYFTang
Copy link
Collaborator Author

Please review and give me your feedback before the TC39 Sept 2023 in 9/26. I would like to get consensus to merge that in TG1

FrankYFTang added a commit to FrankYFTang/test262 that referenced this pull request Sep 15, 2023
@FrankYFTang
Copy link
Collaborator Author

test tc39/test262#3924

@FrankYFTang
Copy link
Collaborator Author

Consensus reach in 2023-09-26 TC39 meeting.

@FrankYFTang FrankYFTang merged commit 7ae05c8 into main Sep 26, 2023
@FrankYFTang FrankYFTang deleted the firstDayOfWeek branch September 26, 2023 05:14
@trflynn89
Copy link

trflynn89 commented Oct 2, 2023

Hello - I'm implementing this change over in Serenity's LibJS and had a question. Do the AO definitions of WeekdayToNumber and WeekdayToString seem backwards? Currently WeekdayToNumber returns a string, and WeekdayToString returns a number, which seems pretty confusing to me.

@trflynn89
Copy link

trflynn89 commented Oct 2, 2023

It also seems like it's possible to hit the Assert: Should not reach here. step in WeekdayToNumber if we provide an invalid value as a -u-fw extension.

For example, if the user provides en-u-fw-100, then in step 34 of Intl.Locale:

34. Let r be ! ApplyUnicodeExtensionToTag(tag, opt, relevantExtensionKeys).

Then r.[[fw]] will have a value of 100. We will pass that to WeekdayToNumber and fail that assertion.

@FrankYFTang
Copy link
Collaborator Author

That is a good point. File a different issue #78 Let's move the discussion to that issue.

ptomato pushed a commit to FrankYFTang/test262 that referenced this pull request Nov 11, 2023
ptomato pushed a commit to FrankYFTang/test262 that referenced this pull request Nov 11, 2023
ptomato pushed a commit to tc39/test262 that referenced this pull request Nov 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants