Skip to content

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

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
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

jira-pull-request-webhook bot commented Aug 2, 2024

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

jira-pull-request-webhook bot commented Aug 2, 2024

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

jira-pull-request-webhook bot commented Aug 9, 2024

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

catamorphism commented Sep 12, 2024

cc @echeran @markusicu

@catamorphism
Copy link
Contributor Author

catamorphism commented Sep 12, 2024

also cc @srl295

@srl295 srl295 requested review from markusicu and srl295 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

catamorphism commented Sep 17, 2024

@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

richgillam commented Sep 18, 2024

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

catamorphism commented Sep 18, 2024

@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

jira-pull-request-webhook bot commented Sep 18, 2024

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

markusicu commented Sep 18, 2024

@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

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 <markus.icu@gmail.com>
@jira-pull-request-webhook
Copy link

jira-pull-request-webhook bot commented Sep 19, 2024

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
@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