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

Be less opinionated in HourCycle docs #6051

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Be less opinionated in HourCycle docs #6051

wants to merge 2 commits into from

Conversation

sffc
Copy link
Member

@sffc sffc commented Jan 31, 2025

Just got to reviewing #6038. I think the changes are good, but I think stylistically they are a bit too opinionated. We normally write our docs to be more matter-of-fact, leaving editorials to tutorials and blog posts.

@sffc sffc requested a review from hsivonen January 31, 2025 12:14
@sffc sffc removed the request for review from zbraniecki January 31, 2025 12:14
("h23" => H23),
/// Variant of the 12-hour clock only used in Japan. Hour system using 0–11; corresponds to 'K' in patterns.
/// Variant of the 12-hour clock. Common in Japan. Hours are numbered 0–11. Corresponds to 'K' in patterns.
Copy link
Member

Choose a reason for hiding this comment

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

CLDR says that JP prefers H over K. Is "common" in Japan a correct characterization? As far as CLDR as a data source goes, "Variant of the 12-hour clock only used in Japan." is factually correct. Why not say that?

If that's not OK, how about "Variant of the 12-hour clock only used in Japan according to CLDR as of February 2025."?

Copy link
Member Author

Choose a reason for hiding this comment

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

My anecdotal experience suggests it is fairly common in Japan. But I'm okay removing the word "common". How's this?

("h11" => H11),
/// __Not actually in use!__ See `H23` above for the 24-hour clock! Included for theoretical completeness. Hour system using 1–24; corresponds to 'k' in pattern.
/// Variant of the 24-hour clock. Not in common use. Hours are numbered 1–24. Corresponds to 'k' in patterns.
Copy link
Member

Choose a reason for hiding this comment

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

I have seen an actual bug arise from this option existing, so I think it's important to give very obvious "Do not use" signals in docs. When the 12-hour clock is h12, it's easy to make the mistake to assume that the 24-hour clock is h24, but that's actually not the case and, moreover, h24 isn't attested to be in use in any locale according to CLDR.

So I think the proposed change makes h24 look way too legitimate to use relative to what the state of CLDR indicates.

Would you be OK with "This is not the typical 24-hour clock; see H23 above for the 24-hour clock! As of February 2025, this variant is not used in any region known to CLDR."

Copy link
Member Author

Choose a reason for hiding this comment

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

How's this?

We use the ❗ emoji elsewhere for this purpose; see https://unicode-org.github.io/icu4x/rustdoc/icu/datetime/pattern/index.html

I'm also open to the idea of renaming this to LegacyH24 or something like that, if we want to take a harder stand. We'd want to pass that by the CLDR Design WG first.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sffc sffc requested a review from hsivonen February 11, 2025 09:25
Copy link
Member

@hsivonen hsivonen left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

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.

2 participants