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

Move icu::calendar::week to experimental #6157

Open
sffc opened this issue Feb 19, 2025 · 7 comments
Open

Move icu::calendar::week to experimental #6157

sffc opened this issue Feb 19, 2025 · 7 comments
Labels
C-datetime Component: datetime, calendars, time zones

Comments

@sffc
Copy link
Member

sffc commented Feb 19, 2025

Given https://unicode-org.atlassian.net/browse/CLDR-18275, it is unclear that this code in its current state is what we will need. I think we should mark it as experimental.

@sffc sffc added the C-datetime Component: datetime, calendars, time zones label Feb 19, 2025
@sffc sffc added this to the ICU4X 2.0 ⟨P1⟩ milestone Feb 19, 2025
@robertbastian
Copy link
Member

This code is also used to determine the first day of the week, and the weekend. Only the min_week_days field, and the week_of_month and week_of_year methods fall under CLDR-18275.

@sffc
Copy link
Member Author

sffc commented Mar 5, 2025

Yeah. So maybe only move the "bad parts" to experimental.

@robertbastian
Copy link
Member

They can't be moved, they're all on the same type. Unless you want to remove the min_week_days field from the data struct, and make a new data struct in experimental which just contains a single u8, and requires the existing data struct to implement week_of_month and week_of_year.

We can just make those methods private if you don't want them to be stable in 2.0

@sffc
Copy link
Member Author

sffc commented Mar 6, 2025

min_week_days is such a small amount of data that I'm not inclined to spend a lot of time on removing it right now.

But I think we should hide the methods behind an experimental feature. Or private, but probably experimental.

@robertbastian
Copy link
Member

I don't think this justifies adding an experimental feature. They are not experimental, they work, and they have been stable for multiple releases. If we say they are footguns, we should remove them until a client asks for them.

@sffc
Copy link
Member Author

sffc commented Mar 6, 2025

I don't think this justifies adding an experimental feature.

I think having an experimental feature should be seen as having a very low cost. It isn't something that should factor very highly into our calculus

They are not experimental, they work, and they have been stable for multiple releases.

And CLDR plans to deprecate this functionality in favor of a different algorithm

If we say they are footguns

They aren't footguns; they are soon but not yet deprecated

we should remove them until a client asks for them.

I would approve a PR removing them. But then we should probably also remove the field from the data struct.

@robertbastian
Copy link
Member

I think having an experimental feature should be seen as having a very low cost.

Ideally we would have experimental code in icu_experimental, not in mature crates behind experimental features.

They aren't footguns; they are soon but not yet deprecated

CLDR is talking about deprecation because they are footguns:

Research, posted in CLDR-17095, suggests that users are surprised when changing their locale changes their week numbering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-datetime Component: datetime, calendars, time zones
Projects
None yet
Development

No branches or pull requests

2 participants