Repository navigation
Inflection-213 Support building and testing with ICU 79 - #214
Conversation
| MessageFormatter::Builder builder(testError); | ||
| builder.setFunctionRegistry(hasSpeak ? ssmlRegistry : plainRegistry) | ||
| .setLocale(icuLocale) | ||
| .setBidiIsolationStrategy(icu::U_MF_BIDI_OFF) |
There was a problem hiding this comment.
This seems interesting. It's a common problem. What's this about? There seems to be a lot of changes related to this option.
There was a problem hiding this comment.
ICU 79 (ICU-22939 / ICU-23143) turned on U_MF_BIDI_AUTO by default. Any placeholder with U_MF_DIRECTIONALITY_UNKNOWN now gets wrapped in U+2068 (FSI) / U+2069 (PDI) isolates, even in LTR locales.
InflectionFunctionValue in MF2Factory.cpp sets inputDir and dir from context.getDirection() and locale.isRightToLeft() (same as ICU's :number and :datetime) so :i:* functions report the locale directionality.
8 XML test cases in de_DE.xml, en_US.xml, and es_MX.xml had bare {$object} / {$name} variables (which default to :string with UNKNOWN directionality and got wrapped in FSI/PDI). Added :i:inflect to those 8 placeholders to match the rest of the XML tests and removed .setBidiIsolationStrategy(icu::U_MF_BIDI_OFF) from Message2Test.cpp.
There was a problem hiding this comment.
Alternatively, instead of annotating those 8 bare {$object} / {$name} placeholders with :i:inflect in <source>, <print> can include the expected ⁨ / ⁩ isolates directly, e.g.:
<source>You opened {$object} and {$object2 :i:inflect}.</source>
<print>You opened ⁨something⁩ and something.</print>That keeps the original bare-variable test inputs and verifies both behaviors side-by-side ({$object} gets isolated while {$object2 :i:inflect} does not). WDYT?
There was a problem hiding this comment.
I'm not really enjoying the extra syntax with i:inflect for plain strings that are not actually inflecting. It's extra writing that has no visible advantage for the average message writer. It should be simple. I'd rather go with the default behavior, and leave the bare strings alone.
There was a problem hiding this comment.
Ok, then I implemented the other option - no more i:inflect, but expectation has to spell out the bidi characters that MF2.0 now inserts.
There was a problem hiding this comment.
Another option is to just turn it off - which George and I agreed on.
grhoten
left a comment
There was a problem hiding this comment.
Since the test files generally work the same, and the API header changes are adapting to the new ICU implementation, these changes seem good.
| .adoptFunction(FunctionName("i:quantify"), inflection::message2::MF2Factory::createQuantifyFunction(mode), ec) | ||
| .adoptFunction(FunctionName("i:list"), inflection::message2::MF2Factory::createListFunction(mode), ec) | ||
| .adoptFunction(FunctionName("i:pronoun"), inflection::message2::MF2Factory::createPronounFunction(mode), ec) | ||
| .build(); |
There was a problem hiding this comment.
At first, I was worried about this change. Since the tests pass unmodified, and I believe that both the formatters and selectors are tested, this change should be fine.
Fixes #213
Summary
Upgrades
inflectionto build and test against ICU 79 (CLDR 49 / Unicode 18), bumpingICU_VERSION_MINIMUMincmake/versions.mkto79.1.Note
The GitHub Actions workflows currently set
ICU_MINOR: '1rc'(release-79.1rc) so CI can run prior to the final ICU79.1release on Wednesday, at which pointICU_MINORwill be updated to'1'.Changes
ICU-23266):MF2Factory.hpp,MF2Factory.cpp, andMessage2Test.cppto use ICU 79's unifiedFunction,FunctionValue, andFunctionContextinterfaces (.adoptFunction(...)onMFFunctionRegistry::Builder, integer index preferences inFunctionValue::selectKeys, andconst FunctionOptionsMap&fromFunctionOptions::getOptions()).ICU-23143):InflectionFunctionValuefrom theFunctionContextlocale/direction, and configure.setBidiIsolationStrategy(icu::U_MF_BIDI_OFF)inMessage2Test#testLocaleswhere XML test expectations do not includeU+2068/U+2069BiDi isolates.ListTest&QuantifyTest):and) list expectations inListTest#testListHi(औरwithout preceding serial comma in CLDR 49).#if U_ICU_VERSION_MAJOR_NUM <= 78workarounds inQuantifyTest#testSerboCroatian.generateICUWrapper(U_LIFETIME_BOUND):\s*\bU_LIFETIME_BOUND\binRunOnFile'snormalizationMapintools/generateICUWrapper/generateICUWrapper.cpp.ICU_VERSION_MINIMUM := 79.1incmake/versions.mk.Ubuntu24.04-x64ICU 79 release archives and installlldincreate-distribution-packaging.yml.cmake-multi-platform.ymlandcreate-distribution-packaging.ymlfrom Homebrewicu4cto building and caching ICU from the official release source archive (icu4c-${ICU_MAJOR}.${ICU_MINOR}-sources.tgz).