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

Discuss: Should LocalePreferences have public fields #5785

Open
zbraniecki opened this issue Nov 6, 2024 · 4 comments
Open

Discuss: Should LocalePreferences have public fields #5785

zbraniecki opened this issue Nov 6, 2024 · 4 comments
Labels
C-locale Component: Locale identifiers, BCP47

Comments

@zbraniecki
Copy link
Member

Issue: Not a fan of this having public fields. But that can be fixed after the PR lands

Originally posted by @sffc in #5729 (comment)

@sffc
Copy link
Member

sffc commented Nov 6, 2024

Duplicate of #5786

@sffc sffc marked this as a duplicate of #5786 Nov 6, 2024
@sffc sffc closed this as not planned Won't fix, can't repro, duplicate, stale Nov 6, 2024
@sffc
Copy link
Member

sffc commented Nov 6, 2024

Actually let's keep this issue open so we have one discussion per thread.

@sffc sffc reopened this Nov 6, 2024
@sffc sffc added C-locale Component: Locale identifiers, BCP47 needs-approval One or more stakeholders need to approve proposal labels Nov 6, 2024
@sffc sffc added this to the ICU4X 2.0 ⟨P1⟩ milestone Nov 6, 2024
@sffc
Copy link
Member

sffc commented Nov 6, 2024

So the main problem here is that DataLocale lives in icu_provider and it needs to be efficiently constructed from this type that lives in icu_locale_core.

Can we just move DataLocale into icu_locale_core?

My personal opinion is that I think it is silly and over-designed to try and keep the types exported from core crates as minimal as possible and put specialized types into other crates. I prefer "useful" core crates: ones that export types that its dependents need.

@sffc
Copy link
Member

sffc commented Nov 7, 2024

Conclusion:

  • LocalePreferences gets private fields
  • It has conversion impls from &LanguageIdentifier and &Locale
  • DataLocale moves to icu_locale_core::DataLocale
  • LocalePreferences has two conversion functions into a DataLocale: to_data_locale_language_priority(self), to_data_locale_region_priority(self)
  • DataLocale gets private fields
  • Add a helper function in icu_provider::marker to call the right DataLocale constructor based on the marker
  • Also change the associated trait function DataMarker::bind to be a helper function in that module
  • Take a birds-eye view of preferences after it is landed in all the crates
    • open question: should preferences be Copy

LGTM: @Manishearth @robertbastian @sffc @zbraniecki

@sffc sffc removed the needs-approval One or more stakeholders need to approve proposal label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-locale Component: Locale identifiers, BCP47
Projects
None yet
Development

No branches or pull requests

2 participants