Full transliterator datagen #5712
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robertbastian
requested review from
Manishearth,
sffc and
a team
as code owners
robertbastian
force-pushed
the
zv
branch
3 times, most recently
from
8f20633 to
db3f275
Compare
sffc
previously approved these changes
Oct 19, 2024
| @@ -65,7 +66,7 @@ impl<'de> serde::Deserialize<'de> for RuleBasedTransliterator<'de> { | |||
| #[serde(borrow)] | |||
| pub id_group_list: VarZeroVec<'a, VarZeroSlice<SimpleIdULE>>, | |||
| #[serde(borrow)] | |||
| pub rule_group_list: VarZeroVec<'a, VarZeroSlice<RuleULE>>, | |||
| pub rule_group_list: VarZeroVec<'a, VarZeroSlice<RuleULE, Index32>, Index32>, | |||
There was a problem hiding this comment.
Nit: probably only 1 of these needs to be Index32.
It looks like the biggest are Hant to Hans, Hans to Hant, and Hani to Latn.
There was a problem hiding this comment.
The inner one needs to be Index32, and therefore the outer one needs to be as well.
| @@ -39,15 +39,57 @@ impl CldrCache { | |||
| continue; | |||
| }; | |||
|
|
|||
| if transform == "und-Ethi-t-und-latn-m0-beta_metsehaf-geminate" { | |||
There was a problem hiding this comment.
Nit: We should have issues for all these with a TODO in the code. One big issue is probably fine.
Comment on lines
+241
to
+242
| #[cfg(feature = "compiled_data")] | ||
| pub fn try_new(locale: &Locale) -> Result<Self, DataError> { |
robertbastian
force-pushed
the
zv
branch
3 times, most recently
from
e603a52 to
12dab67
Compare
sffc
approved these changes
Oct 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CLDR issues
Han-Latin-Names.json should have alias
Han-Latin-Names, currently it just has source (Han) and target (Latin), and the alias derived from that clashes withHan-Latin.jsonde-ASCII.xml refers to a
Any-ASCIItransform, which is not defined in CLDR or UTS-35. This issue was raised on the original ticket: CLDR-10092Thai-Latin.xml refers to a
Any-BreakInternaltransform, which is not defined in CLDR or UTS-35und-Ethi-t-und-latn-m0-beta_metsehaf-geminate.xml refers to a
Ethiopic-Latin/BetaMetsehaftransform which is not defined.am-Ethi-t-d0-morse.xml uses unescaped
$in Unicode set, this is not compatible with the variable semantics introduced in CLDR-16811byn-Ethi-t-byn-latn-m0-xaleget.xml does not currently parse in ICU4X, the error messages is an invalid back reference. Need to investigate
Han-Spacedhan.xml defines a variable as a sequence of two Unicode sets, when probably a union is intended. This doesn't currently parse in ICU4X, and is therefore probably incompatible with the semantics introduced in CLDR-16811
Multiple transliterators use non-canonical property casing, which ICU4X does not support
[:script=foo:]instead of[:Script=foo:]or[:sc=foo:][:ideographic:]instead of[:Ideographic:][:case-ignorable:]instead of[:Case_Ignorable:]Filed CLDR-18043 for the rest