Test collator fallback overrides#7879
Conversation
robertbastian
requested review from
a team,
Manishearth and
sffc
as code owners
robertbastian
force-pushed
the
sr
branch
3 times, most recently
from
47f1dc5 to
2edce4f
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the collation provider to use a macro for data loading and conversion, and updates the ParentLocales struct to use HashMap instead of BTreeMap. I have identified two significant issues: first, the inclusion of CollationRootV1 and CollationTailoringV1 in the collation_provider! macro introduces logic errors regarding locale handling that require either manual implementation or macro extension. Second, the #[cfg(any(feature = "use_wasm", feature = "use_icu4c"))] attribute on several convert methods is overly restrictive and should be removed to prevent unnecessary compilation failures.
|
This is a big diff and I don't think this PR blocks anything so it's not currently at the top of my priority queue, but I will get to it when I have time. |
|
You asked me to do this as part of #7867, would you have also moved that to the back of your "priority queue" if I had done it in that PR? Also, +116 -124 is really not a big diff. |
A big diff like this would have slowed down my review of #7867, yes. It's moderate in line count, but it requires me to upload the structure of the whole file to understand what the new macro does and doesn't accomplish. I just need some focus time to give this PR a review, which I expect to have soon. |
| let parent = parent.into(); | ||
|
|
||
| $( | ||
| if !$marker::INFO.is_singleton { |
There was a problem hiding this comment.
Nit (optional): Since this depends only on $marker, I would prefer if this function were not inside a macro, and the macro can call it, like check_fallback::<$marker>()
| #[serde(rename = "parentLocale")] | ||
| pub(crate) parent_locale: HashMap<LanguageIdentifier, LanguageIdentifier>, | ||
| pub(crate) collations: BTreeMap<String, LanguageIdentifier>, | ||
| pub(crate) collations: HashMap<LanguageIdentifier, LanguageIdentifier>, |
There was a problem hiding this comment.
Observation: the HashMap could make the test nondeterministic if it starts failing, but right now it is fine because the test is passing
2 participants
Fixes #7873
Changelog
N/A