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: Fix WeekInfoOfLocale #83

Merged
merged 10 commits into from
Jul 30, 2024
Merged

Normative: Fix WeekInfoOfLocale #83

merged 10 commits into from
Jul 30, 2024

Conversation

FrankYFTang
Copy link
Collaborator

@FrankYFTang FrankYFTang commented Jul 9, 2024

Fix #82 so so firstDay will return number

@anba @sffc @ben-allen @trflynn89

@FrankYFTang FrankYFTang changed the title Normative: Fix WeekInfoOfLocale so firstDay will return number Normative: Fix WeekInfoOfLocale Jul 9, 2024
Copy link

@trflynn89 trflynn89 left a comment

Choose a reason for hiding this comment

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

Thanks! I verified that with these changes (and with the [[FirstDay]] fix mentioned below), LibJS passes the firstDayOfWeek test262 tests.

locale.html Outdated
1. Let _fw_ be _loc_.[[FirstDayOfWeek]].
1. Let _ca_ be _loc_.[[Calendar]].
1. If _ca_ is *"iso8601"*, then
1. Set _r_.[[FirstDay]] to *"mon"*.

Choose a reason for hiding this comment

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

[[FirstDay]] is an integer - should this be set to 1 instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you are right

1. Set _r_.[[FirstDay]] to *"mon"*.
1. Set _r_.[[MinimalDays]] to *4*.
1. Let _fws_ be _loc_.[[FirstDayOfWeek]].
1. Let _fw_ be !StringToWeekdayValue(_fws_).

Choose a reason for hiding this comment

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

Is the ! necessary for calling an AO that cannot throw?

@FrankYFTang
Copy link
Collaborator Author

@anba @sffc @ben-allen @trflynn89 It will be nice if you all can review and comments before our July 18 , 2024 Thursday TC39 TG 2 meeting.

locale.html Show resolved Hide resolved
locale.html Outdated
1. Let _r_ be a record whose fields are defined by <emu-xref href="#table-locale-weekinfo-record"></emu-xref>, with values based on _locale_.
1. Let _fw_ be _loc_.[[FirstDayOfWeek]].
1. Let _languageId_ be the longest prefix of _locale_ matched by the <code>unicode_language_id</code> Unicode locale nonterminal.
1. Let _r_ be a record whose fields are defined by <emu-xref href="#table-locale-weekinfo-record"></emu-xref>, with values based on _languageId_.
Copy link
Contributor

Choose a reason for hiding this comment

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

locale.html Outdated
@@ -172,8 +195,14 @@ <h1>WeekInfoOfLocale ( _loc_ )</h1>
<emu-alg>
1. Let _locale_ be _loc_.[[Locale]].
1. Assert: _locale_ matches the `unicode_locale_id` production.
1. Let _r_ be a record whose fields are defined by <emu-xref href="#table-locale-weekinfo-record"></emu-xref>, with values based on _locale_.
1. Let _fw_ be _loc_.[[FirstDayOfWeek]].
1. Let _languageId_ be the longest prefix of _locale_ matched by the <code>unicode_language_id</code> Unicode locale nonterminal.
Copy link
Contributor

Choose a reason for hiding this comment

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

First-day-of-week could still depend on -u-ca, I think.

@FrankYFTang
Copy link
Collaborator Author

@anba in the 2024-07-18 TG2 we discuss and people express concern about the patch you proposed. So I will revert that patch from this PR and open another issue to discuss that part for our next meeting

@FrankYFTang FrankYFTang requested a review from sffc July 18, 2024 19:24
@FrankYFTang
Copy link
Collaborator Author

I revert @anba's suggestion from the branch and force push it to this PR so the current PR is without that part.

locale.html Outdated Show resolved Hide resolved
@FrankYFTang
Copy link
Collaborator Author

Remove the "Also fix #30 to explicit set firstDay to "mon" and and minimumDays to 4 if "ca" is "iso8601" " from comments and remove the 8601 handling since it may conflict with the rg issue.

@FrankYFTang FrankYFTang merged commit 04039b8 into main Jul 30, 2024
@FrankYFTang FrankYFTang deleted the fixWeekInfoOfLocale branch July 30, 2024 17:42
@FrankYFTang
Copy link
Collaborator Author

The PR reach consensus in TG1 2024-7-30 . update the PR and keep the ca / rg resolution vague so the action will not conflict wtih rg. merged

@FrankYFTang
Copy link
Collaborator Author

we may have an ICU bug need to fix about the rg/ca resolution to reflect example on https://unicode.org/reports/tr35/tr35-dates.html#first-day-overrides
explicitly
what
en-AU-u-ca-iso8601-rg-afzzzz-sd-cabc
en-AU-u-ca-iso8601-rg-uszzzz-sd-cabc

should return

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.

WeekInfoOfLocale need to convert loc.[[FirstDayOfWeek]] to number before return
4 participants