Add Factory for MF2 ( issues/117 ) #114
Conversation
|
ok, the build problem with the ubuntu build breakage is because it is using 74.2, which does not have MF2 api libicu-dev is already the newest version (74.2-1ubuntu3.1). |
|
Per inflection WG meeting 2025-05-27 Frank will remove the TEST_CASE("MF2Factory#testFormaterFromDataDriven") |
grhoten
left a comment
grhoten
left a comment
There was a problem hiding this comment.
Is there a way to use just the C API from ICU? This library has gone through extensive effort to avoid the C++ API since the C++ ABI is not stable, which is why the ULocale class exists and the icu4cxx wrappers exist. This library is not integrated into ICU yet. Some people still have trouble deciding between using libstdc++ or libc++ in large diverse environments.
I recognize that the MF C API may not exist yet. So that may be an impossible request at this time.
| icu::message2::FormatterFactory* MF2Factory::CreateFormatterFactory() { | ||
| return new InflectionFormatterFactory(); | ||
| } | ||
|
|
||
| icu::message2::SelectorFactory* MF2Factory::CreateSelectorFactory() { | ||
| return new InflectionSelectorFactory(); | ||
| } |
There was a problem hiding this comment.
After thinking about this in more detail, what is the value in these factory methods? It's not caching the factory. There are no complicated data structures. The caller has to delete the factory classes.
Should InflectionFormatterFactory and InflectionSelectorFactory be exposed and allow people to stack allocate them?
There was a problem hiding this comment.
The value is to hide unnecessary details of which exact concrete class implement the FormatterFactory and SelectorFactory so we do not need to expose unnecessary class.
Should InflectionFormatterFactory and InflectionSelectorFactory be exposed and allow people to stack allocate them?
No, because they need to passed into
MFFunctionRegistry::Builder::adoptFormatter and adoptSelector and own by the Builder. Notice in the test code, no code free them. The code free them is inside MFFunctionRegistry::Builder.
There was a problem hiding this comment.
I went to ICU TC and ask them about C API (Richard is also there). The answer is currently they are waiting for the C++ and Java API to be proven stable first. Then they will consider adding a C API later.
Also add unit test and data driven unit test Test data based on inflection/test/resources/inflection/dialog/inflection/*.xml Test formatter while the <result> is not empty Test Selector while the <result> is empty, and there are one attribute which is not "exists". Uppercase the factory function Change copyright, remove iostream, add comments Fix build issue if the icu is < 77 Remove Data Driven test Fix Locale building Remove version Check
FrankYFTang
force-pushed
the
AddMF2Factory
branch
from
8642640 to
bdc9663
Compare
|
PTAL. I address all the review comments. Working on another PR for the data driven test now. |
|
@grhoten Are you ok with this PR? |
* Add Factory for MF2 Also add unit test and data driven unit test Test data based on inflection/test/resources/inflection/dialog/inflection/*.xml Test formatter while the <result> is not empty Test Selector while the <result> is empty, and there are one attribute which is not "exists". Uppercase the factory function Change copyright, remove iostream, add comments Fix build issue if the icu is < 77 Remove Data Driven test Fix Locale building Remove version Check * Reformat
2 participants
Address #117
Add one header file with two functions to create
icu::message2::FormatFactory and
icu::message2::SelectorFactory
Also add unit test and data driven unit test
Test data based on
inflection/test/resources/inflection/dialog/inflection/*.xml
Test formatter while the is not empty
Test Selector while the is empty, and there are one attribute
which is not "exists".