ICU-22261 Add tech preview implementation for MessageFormat 2.0 to icu4c #2579
Conversation
catamorphism
force-pushed
the
for-review
branch
from
19a51f3 to
7be0121
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
7be0121 to
f74fd31
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
f74fd31 to
180077c
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
180077c to
400ad8e
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
400ad8e to
8b8e1bf
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
8b8e1bf to
63cf7fd
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
63cf7fd to
77f7168
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
77f7168 to
40261fa
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
40261fa to
4bfd80e
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
4bfd80e to
0c31a9d
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
0c31a9d to
e8fb455
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
e8fb455 to
67b600f
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
67b600f to
07db6bf
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
07db6bf to
99d5f5b
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
99d5f5b to
9d7cdbb
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
catamorphism
force-pushed
the
for-review
branch
from
ff92f03 to
8dee586
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
|
Thanks @richgillam ! I rebased (which required me to force-push -- hopefully that won't cause problems, since there are no line comments yet to lose). Hopefully that fixes the |
Figured it out and fixed it in b07d4b5. |
We do want to get this into 75 as a tech preview, and we do want to tag the 75 branch asap, so that we can work on getting the release candidate out of the door. So for review, I know this is large, the most important part right now is whether the public API looks ok with spelling, visibility, constness and such. Maybe even try to look at the public parts next to the design doc. And maybe @roubert could take a look at what types cross the DLL boundary, modulo the discussion we had (and documented in the design doc) about std::variant generating exception-throwing code even though the MF2 code does not call those functions. @echeran and maybe @mihnita should probably look over the test code to see if anything looks off. For the bulk of the code, not sure what to say. Large PRs are hard. We probably need to rubber-stamp a lot of it for now. We can still fix some bugs between release candidate and final (in follow-up PRs), but we should not change the API between rc and final. |
Sorry, looks like force-pushing did cause problems (I see that some of the comments have disappeared even though they were not line comments). I'll try to avoid doing that again. |
|
The good news is that all of the CI checks are passing, except for the two that will keep complaining until you squash your commits -- but don't squash until the reviewers have approved. |
|
Can y'all please resolve the remaining comments -- looks like Tim took care of them as much as possible for now -- and move to getting this merged by tomorrow=Wednesday noon Pacific Time? Some bug fixes can go into the maintenance branch, and more changes can go into ICU 76. |
|
As far as I could tell there was nothing actionable in the remaining open comments, so I resolved them, but feel free to reopen @mihnita if there's anything in there that you still think I need to address. |
|
@echeran ok to squash and then merge? |
There was a problem hiding this comment.
If I should offer a very high-level opinion on this, it's that it might be a very good idea to in addition to the already existing UCONFIG_NO_FORMATTING flag also add one such flag specifically for this new MF2 functionality, to make it easy to build ICU4C without any of it, for those who won't be using this tech preview and would prefer to not have tens of thousands of lines of new mostly unreviewed code included in their binaries.
Good idea! |
I would be happy to do that in a follow-up PR. Note that as the code author, I am just as nervous about shipping unreviewed code as users might be about including that code in their binaries :) |
Yep, let's squash & merge now. |
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
|
(note: For anyone noticing the PR checker bot claiming that the branch contents changed across the force push: I don't think that is the case. I did the squash locally since I cannot use the PR checker bot to squash on another person's PR branch (PR checker bot does not have permissions to do so). When I run |
|
Thanks everyone!! 🎉 |
Please do now :-) |
Submitted: #2931 |
This pull request implements the draft MessageFormat 2.0 spec; the goal is to implement the same functionality as the existing ICU4J tech preview for MessageFormat 2.0, though there are differences since the code in this pull request reflects a newer version of the spec.
A design document has been shared with the icu-design mailing list, with the deadline for commenting as 2023-09-05, though this may be extended if not enough feedback is received. There are some open comments on the design document that I'll address, but none of them seem to be a blocker for initial review of this draft pull request.
Checklist