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: Change the hourCycle default logic #758

Merged
merged 8 commits into from
Sep 27, 2023
Merged

Conversation

FrankYFTang
Copy link
Contributor

@FrankYFTang FrankYFTang commented Mar 13, 2023

While hour12 is false, let hourCycle to be alwasyt 'h23' but not 'h24'.

The current logic is not reasonable. We see no region in the CLDR use h24 hour cycle as default. While we set a hour12: false on a 12 hour system region (which use 1:00 - 12:59), we should set to h23 instead of h24

We need to make this change
so while hour12 is false on 'en' locale, it will be set to h23 instead of h24
but also while hour12 is true on 'ja' locale, it will be set to h11 but not h12.

Address #402

While hour12 is either true or false, let hourCycle to be either 'h12' or 'h23' but not 'h11' nor 'h24'.

The current logic is not reasonable. We see no region in the CLDR use h11 nor h24 hour cycle as default. 
While we set a hour12: true on a 24 hour system region (which use 0:00 - 23:59), we should set to h12 instead of h11

Do not perform the starnge logic of setting default hourCycle based on both hour12 and the defaultHourCycle while hour12 is true or false. Instead, only depends on hour12 to set it to h12 or h23. 

Address #402
@FrankYFTang
Copy link
Contributor Author

notice grep "<hours.*allowed" ./cldr/common/supplemental/supplementalData.xml show possibility of h H or K but no possibility of k in the latest CLDR and it is unlikely that will happen in the future from my understanding.

spec/datetimeformat.html Outdated Show resolved Hide resolved
spec/datetimeformat.html Outdated Show resolved Hide resolved
@FrankYFTang
Copy link
Contributor Author

PTAL I took anba's suggestion and make it data driven.

spec/datetimeformat.html Outdated Show resolved Hide resolved
spec/datetimeformat.html Outdated Show resolved Hide resolved
FrankYFTang and others added 2 commits April 5, 2023 17:53
Co-authored-by: Shane F. Carr <sffc@google.com>
Co-authored-by: Shane F. Carr <sffc@google.com>
@FrankYFTang FrankYFTang requested a review from sffc April 6, 2023 00:53
@sffc
Copy link
Contributor

sffc commented Apr 7, 2023

@ryzokuken ryzokuken added has consensus Has consensus from TC39-TG2 normative labels May 4, 2023
@ryzokuken ryzokuken added the has consensus (TG1) Has consensus from TC39-TG1 label Jun 29, 2023
@FrankYFTang
Copy link
Contributor Author

somehow no one ever present this PR to TG39 in May 2023 or July 2023 meeting. I add it to https://github.com/tc39/agendas/blob/main/2023/09.md

@FrankYFTang
Copy link
Contributor Author

we need test262 tests for this PR

Ms2ger pushed a commit to ben-allen/test262 that referenced this pull request Aug 25, 2023
ctcpip added a commit to ctcpip/tc39-times that referenced this pull request Aug 30, 2023
@FrankYFTang
Copy link
Contributor Author

@anba - any suggetion how should we write test262 test to test this PR?

@anba
Copy link
Contributor

anba commented Sep 14, 2023

@anba - any suggetion how should we write test262 test to test this PR?

https://github.com/tc39/test262/blob/main/test/intl402/DateTimeFormat/prototype/resolvedOptions/hourCycle-default.js needs to be updated to reflect the PR changes. The test is already marked as locale-specific, so we could simply change it to:

let locales = ["en", "fr", "it", "ja", "zh", "ko", "ar", "hi"];

for (let locale of locales) {
  let hcDefault = new Intl.DateTimeFormat(locale, {hour: "numeric"}).resolvedOptions().hourCycle;
  assert(
    hcDefault === "h11" || hcDefault === "h12" || hcDefault === "h23" || hcDefault === "h24",
    "hcDefault is one of [h11, h12, h23, h24]"
  );

  let hour12 = new Intl.DateTimeFormat(locale, {hour: "numeric", hour12: true}).resolvedOptions().hourCycle;
  assert(hour12 === "h11" || hour12 === "h12", "hour12 is one of [h11, h12]");

  let hour24 = new Intl.DateTimeFormat(locale, {hour: "numeric", hour12: false}).resolvedOptions().hourCycle;
  assert(hour24 === "h23" || hour24 === "h24", "hour24 is one of [h23, h24]");

  if (hcDefault === "h11" || hcDefault === "h12") {
    assert.sameValue(hour12, hcDefault, "hour12 matches hcDefault");
  } else {
    assert.sameValue(hour24, hcDefault, "hour24 matches hcDefault");
  }

  // 24-hour clock uses the "h23" format in all locales.
  assert.sameValue(hour24, "h23");

  // 12-hour clock uses the "h12" format in all locales except "ja".
  assert.sameValue(hour12, locale === "ja" ? "h11" : "h12");
}

@ryzokuken
Copy link
Member

@FrankYFTang @anba is merging this PR blocked on the tests?

@ryzokuken
Copy link
Member

2023-09-26: This PR achieved TG1 consensus

@FrankYFTang
Copy link
Contributor Author

FrankYFTang commented Sep 26, 2023

This PR reach consensus on 2023-09-26 meeting

@FrankYFTang
Copy link
Contributor Author

I believe the PR need to merge first before merging the test, right?

@FrankYFTang
Copy link
Contributor Author

@FrankYFTang @anba is merging this PR blocked on the tests?

The test is blocked by the merge of this PR tc39/test262#3885

Copy link
Contributor

@anba anba left a comment

Choose a reason for hiding this comment

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

Two small issues in the description text, otherwise LGTM!

spec/datetimeformat.html Outdated Show resolved Hide resolved
spec/datetimeformat.html Outdated Show resolved Hide resolved
ryzokuken and others added 2 commits September 27, 2023 05:36
Co-authored-by: André Bargull <andre.bargull@gmail.com>
Co-authored-by: André Bargull <andre.bargull@gmail.com>
@ryzokuken ryzokuken merged commit 2f002b2 into master Sep 27, 2023
3 checks passed
@ryzokuken ryzokuken deleted the FrankYFTang-patch-2 branch September 27, 2023 03:38
@jufemaiz
Copy link

🎉

ptomato pushed a commit to ben-allen/test262 that referenced this pull request Oct 2, 2023
ptomato pushed a commit to ben-allen/test262 that referenced this pull request Oct 2, 2023
ptomato pushed a commit to tc39/test262 that referenced this pull request Oct 2, 2023
ben-allen pushed a commit to ben-allen/ecma402 that referenced this pull request Oct 12, 2023
While hour12 is either true or false, let hourCycle to be either 'h12' or 'h23' but not 'h11' nor 'h24'.

The current logic is not reasonable. We see no region in the CLDR use h11 nor h24 hour cycle as default. 
While we set a hour12: true on a 24 hour system region (which use 0:00 - 23:59), we should set to h12 instead of h11

Do not perform the starnge logic of setting default hourCycle based on both hour12 and the defaultHourCycle while hour12 is true or false. Instead, only depends on hour12 to set it to h12 or h23. 

Address tc39#402

Co-authored-by: Shane F. Carr <sffc@google.com>
Co-authored-by: Ujjwal Sharma <ryzokuken@igalia.com>
Co-authored-by: André Bargull <andre.bargull@gmail.com>
@jufemaiz
Copy link

Can I assume that this will form part of a changeset for the 10th edition?

Ref: https://ecma-international.org/publications-and-standards/standards/ecma-402/

hubot pushed a commit to v8/v8 that referenced this pull request Feb 24, 2024
Spec tc39/ecma402#758

Change the hourCycle for not returning h24 while hour12 is false

Bug: v8:14542
Change-Id: I65a6719ed0da1fd43c3ac25bd107c89a25ae7f21
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5319663
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/main@{#92519}
ben-allen added a commit to ben-allen/mdn-content that referenced this pull request Apr 30, 2024
ben-allen added a commit to ben-allen/mdn-content that referenced this pull request May 1, 2024
Josh-Cena added a commit to mdn/content that referenced this pull request May 1, 2024
…option (#33343)

* Update `Intl.DateTimeFormat` to reflect new behaviour of `hourCycle`
option after tc39/ecma402#758

* Update files/en-us/web/javascript/reference/global_objects/intl/datetimeformat/datetimeformat/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update index.md

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus (TG1) Has consensus from TC39-TG1 has consensus Has consensus from TC39-TG2 has tests normative
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants