Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ICU-22762 MF2: Add methods to control error handling behavior #3083

Merged
merged 1 commit into from
Sep 19, 2024
Merged

ICU-22762 MF2: Add methods to control error handling behavior #3083

merged 1 commit into from
Sep 19, 2024

Conversation

catamorphism
Copy link
Contributor

@catamorphism catamorphism commented Aug 2, 2024 •
edited
Loading

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22762
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/messageformat2_errors.h is different
  • icu4c/source/i18n/messageformat2_formatter.cpp is different
  • icu4c/source/i18n/unicode/messageformat2.h is different
  • icu4c/source/test/intltest/messageformat2test.cpp is different
  • icu4c/source/test/intltest/messageformat2test.h is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/messageformat2_formatter.cpp is different
  • icu4c/source/test/intltest/messageformat2test.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/unicode/messageformat2.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@catamorphism catamorphism marked this pull request as ready for review September 12, 2024 15:55
@catamorphism
Copy link
Contributor Author

cc @echeran @markusicu

@catamorphism
Copy link
Contributor Author

also cc @srl295

@srl295 srl295 requested review from srl295 and markusicu September 12, 2024 16:34
richgillam
richgillam previously approved these changes Sep 12, 2024
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOKTM

@catamorphism
Copy link
Contributor Author

@richgillam @echeran Per a suggestion from @mihnita on the design doc, I added 7e421d4 -- all that does is move the enum definition out of the builder class into the MessageFormatter class.

I didn't change the design doc because I didn't want to edit what was already accepted by TC; thus I'm just making this change in the code but calling attention to it being different from the design doc.

@richgillam
Copy link
Contributor

I didn't change the design doc because I didn't want to edit what was already accepted by TC; thus I'm just making this change in the code but calling attention to it being different from the design doc.

At some point, I'm going to have to go through and promote all the new APIs from draft to stable, and part of that process involves verifying that what I'm promoting matches the API proposals that were approved. This will cause hiccups in that process.

I don't object to the actual change, but I think you want to update the proposal to match what you're checking in, and you should probably either go through API review again (hopefully it'd just be a rubber stamp) or at least notify everyone via the icu-design mailing list and give them a chance to object.

@catamorphism
Copy link
Contributor Author

@richgillam:

I don't object to the actual change, but I think you want to update the proposal to match what you're checking in, and you should probably either go through API review again (hopefully it'd just be a rubber stamp) or at least notify everyone via the icu-design mailing list and give them a chance to object.

OK, that makes sense. I changed the design doc to match this code, and emailed the icu-design list.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/messageformat2_data_model.cpp is different
  • icu4c/source/i18n/messageformat2_errors.cpp is different
  • icu4c/source/i18n/messageformat2_errors.h is different
  • icu4c/source/i18n/messageformat2_serializer.cpp is different
  • icu4c/source/i18n/messageformat2.cpp is different
  • icu4c/source/test/intltest/messageformat2test.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@markusicu
Copy link
Member

@richgillam:

I don't object to the actual change, but I think you want to update the proposal to match what you're checking in, and you should probably either go through API review again (hopefully it'd just be a rubber stamp) or at least notify everyone via the icu-design mailing list and give them a chance to object.

OK, that makes sense. I changed the design doc to match this code, and emailed the icu-design list.

I added a note for this into the ICU 76 API proposal status doc.

markusicu
markusicu reviewed Sep 18, 2024
View reviewed changes
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some concerns but I am not going to block merging

icu4c/source/i18n/messageformat2_formatter.cpp Show resolved Hide resolved
icu4c/source/i18n/messageformat2_formatter.cpp Outdated Show resolved Hide resolved
icu4c/source/i18n/messageformat2_formatter.cpp Outdated Show resolved Hide resolved
icu4c/source/i18n/unicode/messageformat2.h Outdated Show resolved Hide resolved
icu4c/source/i18n/unicode/messageformat2.h Outdated Show resolved Hide resolved
icu4c/source/test/intltest/messageformat2test.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/messageformat2test.cpp Outdated Show resolved Hide resolved
richgillam
richgillam approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOKTM

@catamorphism @markusicu
ICU-22762 MF2: Add builder method to control error handling behavior
3cc5232 
Add MessageFormatter::Builder::setErrorHandlingBehavior() method
and a new enum type MessageFormatter::UMFErrorHandlingBehavior
to denote strict or best-effort behavior.

The reason for adding a single method that takes an enum is to allow
for the possibility of more error handling modes in the future.

Co-authored-by: Markus Scherer <[email protected]>
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@catamorphism catamorphism merged commit 23edf9c into unicode-org:main Sep 19, 2024
94 checks passed
@catamorphism catamorphism mentioned this pull request Sep 19, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richgillam richgillam richgillam approved these changes

@echeran echeran Awaiting requested review from echeran

@srl295 srl295 Awaiting requested review from srl295

@markusicu markusicu Awaiting requested review from markusicu

Assignees

@echeran echeran

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@catamorphism @richgillam @markusicu @echeran