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

Remove locales with explicit u-hc Unicode extension #3958

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

anba
Copy link
Contributor

@anba anba commented Nov 9, 2023

The tests are passing an explicit hour12 option, which disables any hourCycle option and any u-hc Unicode extension. Therefore passing input locales with u-hc is invalid.

let locale = "en-u-hc-h24";
let hcDefault = new Intl.DateTimeFormat(locale, { hour: "numeric" }).resolvedOptions().hourCycle;

assert.sameValue(hcDefault, "h24", "hour-cycle through Unicode extension");

let hc24 = new Intl.DateTimeFormat(locale, { hour: "numeric", hour12: false }).resolvedOptions().hourCycle;

// Incorrect assertion, because |hc24| uses |hour12|, which disables any
// hour-cycle option and the hour-cycle is determined from the locale "en".
// That means |hc24| will be "h23".
assert.sameValue(hc24, hcDefault);

@anba anba requested a review from a team as a code owner November 9, 2023 12:49
Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

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

Given:

// no locale has "h24" as a default. see https://github.com/tc39/ecma402/pull/758#issue-1622377292

This comment / code path no longer seems relevant:

// however, "h24" can be set via locale extension.

@ptomato
Copy link
Contributor

ptomato commented Mar 1, 2024

OK. @ben-allen explained this to me and I think I understand it.

With that understanding (which I may still have gotten wrong), Linus's comment makes sense to me. Maybe the loop body should look something like this?

let hcDefault = new Intl.DateTimeFormat(locale, { hour: "numeric" }).resolvedOptions().hourCycle;
let hcHour12 = new Intl.DateTimeFormat(locale, { hour: "numeric", hour12: true }).resolvedOptions().hourCycle;

assert(hcHour12 === "h11" || hcHour12 === "h12", "Expected hourCycle for hour12: true to be in [\"h11\", \"h12\"]");
if (hcDefault === "h11" || hcDefault === "h12") {
  assert.sameValue(hcHour12, hcDefault);
}

// no locale has "h24" as a default. see https://github.com/tc39/ecma402/pull/758#issue-1622377292
let hcNotHour12 = new Intl.DateTimeFormat(locale, { hour: "numeric", hour12: false }).resolvedOptions().hourCycle;
assert.sameValue(hcNotHour12, "h23", "Expected hourCycle for hour12: false to be \"h23\"");

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

(see previous comment, which I neglected to send as a review comment)

@anba
Copy link
Contributor Author

anba commented Mar 8, 2024

I've simply changed the test to match what I've originally proposed in tc39/ecma402#758 (comment).

The tests are passing an explicit `hour12` option, which disables any
`hourCycle` option and any `u-hc` Unicode extension. Therefore passing
input locales with `u-hc` is invalid.

```js
let locale = "en-u-hc-h24";
let hcDefault = new Intl.DateTimeFormat(locale, { hour: "numeric" }).resolvedOptions().hourCycle;

assert.sameValue(hcDefault, "h24", "hour-cycle through Unicode extension");

let hc24 = new Intl.DateTimeFormat(locale, { hour: "numeric", hour12: false }).resolvedOptions().hourCycle;

// Incorrect assertion, because |hc24| uses |hour12|, which disables any
// hour-cycle option and the hour-cycle is determined from the locale "en".
// That means |hc24| will be "h23".
assert.sameValue(hc24, hcDefault);
```
@ptomato
Copy link
Contributor

ptomato commented Mar 9, 2024

OK, fair enough, that seems fine for now.

@ptomato ptomato merged commit 263ed65 into tc39:main Mar 9, 2024
9 checks passed
@anba anba deleted the hourcycle branch June 24, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants