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

Resolution between API options and -u- Locale extensions (migrate constructors to ICU4X Preferences) #419

Closed
sffc opened this issue Dec 10, 2020 · 15 comments
Assignees
Labels
A-design Area: Architecture or design A-tailoring Area: User preferences, locale extensions, tailoring C-meta Component: Relating to ICU4X as a whole S-epic Size: Major project (create smaller child issues) T-core Type: Required functionality

Comments

@sffc
Copy link
Member

sffc commented Dec 10, 2020

ICU4C and ICU4J are inconsistent in honoring -u- extensions. Sometimes the formatters obey them, or not. They also support setters instead of -u-. Can we do something in ICU4X to unify the pattern around the resolution between -u- and API options?

@sffc sffc added C-meta Component: Relating to ICU4X as a whole A-design Area: Architecture or design discuss Discuss at a future ICU4X-SC meeting labels Dec 10, 2020
@sffc sffc added this to the ICU4X 0.2 milestone Dec 10, 2020
@sffc
Copy link
Member Author

sffc commented Jan 21, 2021

@mihnita - Every formatter should look at -u- and obey the parts that are relevant.

@zbraniecki - There should be a common resolution logic helper.

@nciric - We largely deal with this in ECMA-402.

@sffc
Copy link
Member Author

sffc commented Jan 21, 2021

Deliverable: make the shared helper class. A utility. Document in the style guide.

@sffc sffc added T-core Type: Required functionality and removed discuss Discuss at a future ICU4X-SC meeting labels Jan 21, 2021
@sffc sffc assigned zbraniecki and unassigned mihnita Jan 21, 2021
@sffc sffc added the A-tailoring Area: User preferences, locale extensions, tailoring label Feb 27, 2021
@sffc
Copy link
Member Author

sffc commented Mar 12, 2021

Revisit in 0.3

@sffc
Copy link
Member Author

sffc commented Jan 27, 2022

@sffc to draft a design doc.

@sffc sffc removed this from the 2022 Q1 0.6 Sprint A milestone Mar 24, 2022
@sffc sffc added the discuss-priority Discuss at the next ICU4X meeting label Jul 13, 2022
@sffc
Copy link
Member Author

sffc commented Jul 15, 2022

See #2136 for additional discussion.

For ICU4X 1.0 we agreed to move forward with option 3 above, but we will definitely continue to consider adopting Preferences in ICU4X 2.0

@sffc sffc removed the discuss-priority Discuss at the next ICU4X meeting label Jul 15, 2022
@sffc sffc modified the milestones: ICU4X 1.0 (Features), ICU4X 2.0 Jul 15, 2022
@zbraniecki
Copy link
Member

For ICU4X 1.0 we agreed to move forward with option 3 above,

Is that try_new(locale: DataLocale) or try_new<L: Into<DataLocale>>(locale: L) ?

@sffc
Copy link
Member Author

sffc commented Jul 16, 2022

it is try_new(locale: &DataLocale) as laid out in #2136 (comment)

@sffc
Copy link
Member Author

sffc commented Jul 11, 2024

Notes about icu_preferences integration plan:

@sffc sffc changed the title Resolution between API options and -u- Locale extensions Resolution between API options and -u- Locale extensions (migrate constructors to ICU4X Preferences) Sep 17, 2024
@sffc
Copy link
Member Author

sffc commented Sep 17, 2024

Plan:

  • @robertbastian to open a PR migrating either PluralRules, FixedDecimalFormatter, or Collator to icu_preferences by October 1
  • If all looks OK, @zbraniecki to migrate remaining crates during the month of October

@robertbastian
Copy link
Member

robertbastian commented Sep 23, 2024

I've tried to migrate icu_collator and have hit the following road blocks:

  • CollatorPreferences wraps an optional LanguageIdentifier. It is unclear to me what the difference between None and Some(LanguageIdentifier::UND) is. We have so far treated these the same
  • CollatorPreferences wraps an owned LanguageIdentifier. Constructing CollatorPreferences from &LanguageIdentifier or &Locale requires a clone.
  • ResolvedCollatorPreferences does not wrap a DataLocale

@robertbastian
Copy link
Member

My proposal would be:

  • CollatorPreferences borrows a LanguageIdentifier, so it gains a lifetime
  • ResolvedCollatorPreferences stores a DataLocale internally, so it doesn't need a lifetime, can be trivially converted to DataIdentifier, and is Copy.

@robertbastian
Copy link
Member

Another issue: resolving preferences requires data. Where does this data come from?

@sffc
Copy link
Member Author

sffc commented Sep 23, 2024

I feel like ResolvedPreferences is trying to do multiple things at once:

  1. Put together the variables we need for data loading
  2. Return the resolved options to the user

But, these are two separate operations. Only the second one needs data, and the first one needs to keep track of extra things like -u-rg, which is required for region-priority fallback.

@Manishearth
Copy link
Member

Everyone is on Preferences now, this should be done.

@github-project-automation github-project-automation bot moved this from Being worked on to Done in icu4x 2.0 Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-design Area: Architecture or design A-tailoring Area: User preferences, locale extensions, tailoring C-meta Component: Relating to ICU4X as a whole S-epic Size: Major project (create smaller child issues) T-core Type: Required functionality
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants