Use script fallback in transliterator #6542
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @robertbastian, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces a script fallback mechanism in the transliterator. This change modifies the Transliterator struct to include a LocaleFallbacker and updates the data loading process to utilize locale fallback when a specific transliterator is not found. The goal is to improve the transliterator's ability to handle different locales by falling back to a more general script when a specific transliterator is unavailable.
Highlights
- Script Fallback: Implements a script fallback mechanism for transliterators, allowing the system to use a more general transliterator when a specific one is not found.
- Locale Fallbacker: Introduces
LocaleFallbackerto handle locale fallback during transliterator creation. - Data Loading: Modifies the data loading process to utilize locale fallback when a specific transliterator is not found.
- Provider Updates: Updates the
RuleCollectionProviderto include a locale provider and modifies theas_provider_unstablefunction to accept a locale provider.
Changelog
Click here to see the changelog
- components/experimental/Cargo.toml
- Adds
icu_locale/compiled_dataandicu_locale/serdeto thecompiled_dataandserdefeatures respectively.
- Adds
- components/experimental/src/transliterate/compile/mod.rs
- Adds
icu_locale::provider::{LocaleLikelySubtagsLanguageV1, LocaleParentsV1}to the use statements. - Modifies
RuleCollectionProviderto include a locale provider. - Updates
as_provider_unstableto accept a locale provider. - Updates
RuleCollectionProviderstruct to includelocale_providerfield.
- Adds
- components/experimental/src/transliterate/provider.rs
- Adds
icu_locale::fallback::{LocaleFallbackConfig, LocaleFallbackPriority}to the use statements. - Adds
fallback_configto theTransliteratorRulesV1data marker to prioritize script fallback.
- Adds
- components/experimental/src/transliterate/transliterator/mod.rs
- Adds
icu_locale::fallback::LocaleFallbackerandicu_locale::provider::*to the use statements. - Updates
Transliterator::try_new_unstableandTransliterator::try_new_with_override_unstableto accept a locale provider. - Modifies the internal
load_rbtfunction to include locale fallback logic when a specific transliterator is not found.
- Adds
- components/experimental/tests/transliterate/lower_ascii.rs
- Adds
icu_locale::provider::Bakedto theTransliteratorMultiSourceProviderstruct. - Updates
TransliteratorMultiSourceProviderto implementDataProviderforicu_locale::provider::Baked. - Adds locale provider to the
Transliterator::try_new_unstablecall.
- Adds
- provider/source/src/transforms/mod.rs
- Updates
as_provider_unstablecalls to include the locale provider.
- Updates
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A script's gentle fall,
When words seek a kindred soul,
Forms find new embrace.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces script fallback in the transliterator, enhancing its ability to handle locales with missing specific transliteration rules. The changes involve modifications to Cargo.toml, several source files, and tests to incorporate locale fallback logic. Overall, the changes seem well-structured and address the intended functionality.
Summary of Findings
- Missing locale provider in
as_provider_unstable: Theas_provider_unstablefunction incomponents/experimental/src/transliterate/compile/mod.rsneeds to accept a locale provider to align with the updatedRuleCollectionProvider. - Potential clippy warning: The
load_rbtfunction incomponents/experimental/src/transliterate/transliterator/mod.rsis annotated with#[allow(clippy::too_many_arguments)]. Consider refactoring the function or its parameters to reduce the number of arguments if possible.
Merge Readiness
The pull request introduces significant functionality by adding script fallback to the transliterator. While the changes appear to be well-structured, addressing the identified issues is crucial before merging. Specifically, the missing locale provider in as_provider_unstable and the potential refactoring of load_rbt should be considered. I am unable to approve this pull request, and recommend that it not be merged until these issues are addressed and others have reviewed and approved this code.
robertbastian
force-pushed
the
transliterator
branch
from
b98828a to
77faddc
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR updates the transliterator implementation to support a script fallback by adding a new locale provider parameter and adjusting related APIs. Key changes include:
- Modifying function signatures and calls (e.g. as_provider_unstable, try_new_unstable) to pass an extra locale provider.
- Updating tests to include the new parameter and modifying provider configuration and macro definitions accordingly.
- Adjusting Cargo.toml features to include the icu_locale compiled dependencies.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| provider/source/src/transforms/mod.rs | Updated as_provider_unstable invocation with extra parameter. |
| components/experimental/tests/transliterate/lower_ascii.rs | Added new provider argument to test function calls. |
| components/experimental/src/transliterate/transliterator/mod.rs | Added locale_provider support and updated fallback closure usage. |
| components/experimental/src/transliterate/provider.rs | Updated provider configuration with locale fallback. |
| components/experimental/src/transliterate/compile/mod.rs | Extended RuleCollectionProvider to include a locale provider and updated macros. |
| components/experimental/Cargo.toml | Updated features list to include icu_locale and adjust dependencies. |
Comments suppressed due to low confidence (1)
components/experimental/src/transliterate/compile/mod.rs:272
- Ensure consistent generic type parameter naming for the locale provider across the module. The struct uses 'LP' while later impls and macros refer to this parameter as 'NL', which may lead to confusion. Consider using 'LP' uniformly.
pub struct RuleCollectionProvider<'a, PP: ?Sized, NP: ?Sized, NC: ?Sized, LP: ?Sized> {
| } | ||
| } | ||
|
|
||
| redirect!(LocaleParentsV1, LocaleLikelySubtagsLanguageV1); |
There was a problem hiding this comment.
Nit:
- Please split
LocaleParentsV1andLocaleLikelySubtagsLanguageV1into separate invocations of the macro. It reads like you are "redirecting" one to the other, which isn't what is going on at all - Please rewrite all the others to use the macro. Add an argument to take the
locale_providerident
| #[cfg(test)] | ||
| println!("{marker_attributes:?}"); |
| fn load_rbt<PT, PN, PC, F>( | ||
| marker_attributes: &DataMarkerAttributes, | ||
| lookup: Option<&F>, | ||
| transliterator_provider: &PT, | ||
| normalizer_provider: &PN, | ||
| casemap_provider: &PC, | ||
| allow_internal: bool, | ||
| fallbacker: impl Fn() -> Result<LocaleFallbacker, DataError>, |
There was a problem hiding this comment.
Thought: You can't use the fallbacker built into databake because you're falling back within data marker attributes? Maybe worth adding a comment explaining this, since it is unusual ICU4X style to explicitly take a fallbacker in a constructor.
| .allow_identifier_not_found()? | ||
| { | ||
| t.payload | ||
| } else if let Ok(locale) = marker_attributes.parse::<Locale>() { |
There was a problem hiding this comment.
Question: is the request und/de-t-fr-ch-d0-ascii or de/fr-ch-d0-ascii ?
| candidate.extensions.transform.lang = | ||
| Some(source_iterator.get().into_locale().id); | ||
| if let Ok(t) = Self::load_rbt( | ||
| #[allow(clippy::unwrap_used)] // infallible |
There was a problem hiding this comment.
Nit: It isn't an infallible unwrap; the unwrap is safe because the invariants of being ASCII-only are upheld
| candidate.extensions.transform = transform_extensions.clone(); | ||
| candidate.extensions.transform.lang = | ||
| Some(source_iterator.get().into_locale().id); | ||
| if let Ok(t) = Self::load_rbt( |
There was a problem hiding this comment.
Question/Suggestion: why the recursive load_rbt? Should we also be checking the user lookup function? Or should you be calling directly into transliterator_provider.load like before?
Replaces #3972