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 internal fallback option to baked exporter options #5036

Merged
merged 11 commits into from
Jun 12, 2024

Conversation

robertbastian
Copy link
Member

@robertbastian robertbastian commented Jun 11, 2024

I also changed the DeduplicationStrategy to default to RetainBaseLanguages, instead of being inferred by supports_builtin_fallback. I haven't change testdata/bakedata generation, as I don't want a data diff in this PR.

@robertbastian robertbastian requested review from sffc, Manishearth and a team as code owners June 11, 2024 14:05
@Manishearth Manishearth removed their request for review June 11, 2024 15:36
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

LGTM except please change DeduplicationStrategy back to depending on the exporter. I suggest changing the trait function supports_built_in_fallback to instead be default_deduplication_strategy.

@robertbastian robertbastian requested a review from sffc June 12, 2024 10:23
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Nice!

@robertbastian robertbastian requested a review from sffc June 12, 2024 11:52
@robertbastian
Copy link
Member Author

robertbastian commented Jun 12, 2024

The current architecture raises the question: Are FallbackOptions still fallback options, or are they deduplication options? And do we flatten the options and just use the enum, like with_locales_and_deduplication(..., DeduplicationStrategy)? The enum is non-exhaustive, that might be enough. And do we still need with_locales_no_fallback, or is this now the same as FallbackOptions::no_deduplication()?

@sffc
Copy link
Member

sffc commented Jun 12, 2024

The current architecture raises the question: Are FallbackOptions still fallback options, or are they deduplication options? And to flatten the options and just use the enum, like with_locales_and_deduplication(..., DeduplicationStrategy)? The enum is non-exhaustive, that might be enough. And do we still need with_locales_no_fallback, or is this now the same as FallbackOptions::no_deduplication()?

We've gone in circles so many times that I think making things "excessively non-exhaustive" is the right path.

It would be reasonable though to have the following functions:

  • with_preresolved_locales(impl Iterator<Item = LanguageIdentifier>)
  • with_locale_families_optional_runtime_fallback(impl Iterator<Item = LocaleFamily>)
  • with_locale_families_for_runtime_fallback_retain_base_languages(impl Iterator<Item = LocaleFamily>)
  • with_locale_families_for_runtime_fallback_maximal_deduplication(impl Iterator<Item = LocaleFamily>)

@robertbastian
Copy link
Member Author

Different PR anyway

@robertbastian robertbastian merged commit cb7905d into unicode-org:main Jun 12, 2024
28 checks passed
@robertbastian robertbastian deleted the internal branch June 12, 2024 12:45
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