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.

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

ICU-22908 DRAFT!!! MF2 ICU4J: Update spec tests and update implementation for recent spec changes #3206

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mihnita
Copy link
Contributor

@mihnita mihnita commented Sep 22, 2024

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22908
  • 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

@mihnita mihnita changed the title ICU-22908 MF2 ICU4J: Update spec tests and update implementation for recent spec changes ICU-22908 DRAFT!!! MF2 ICU4J: Update spec tests and update implementation for recent spec changes Sep 22, 2024
@mihnita
Copy link
Contributor Author

mihnita commented Sep 22, 2024

Updated the Java code to pass the tests equivalent to #3198

The .json files are not identical, the ones here "bring back" the javaIgnore.
Many of them (close to 60) are about issue 782, which is about error handling (fallback vs strict).

That is implemented, but it is an either / or, with a flag.
And I didn't update the test framework to do that.

TLDR: if we merge the 2 PRs into one, then it should contain these updated .json files, not only the Java code changes.


I still think it is a bad idea to try and push for this soo late in the game.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4j/main/a is now changed in the branch
  • icu4j/main/core/src/main/java/com/ibm/icu/message2/MFSerializer.java is now changed in the branch
  • icu4j/main/core/src/test/java/com/ibm/icu/dev/test/message2/CustomFormatterMessageRefTest.java is now changed in the branch
  • icu4j/main/core/src/test/java/com/ibm/icu/dev/test/message2/CustomFormatterPersonTest.java is now changed in the branch
  • icu4j/main/core/src/test/java/com/ibm/icu/dev/test/message2/MessageFormat2Test.java is now changed in the branch
  • icu4j/main/core/src/test/java/com/ibm/icu/dev/test/message2/Mf2IcuTest.java is now changed in the branch
  • icu4j/main/core/src/test/java/com/ibm/icu/dev/test/message2/SerializationTest.java 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!

  • icu4j/main/a is no longer 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!

  • icu4j/main/common_tests/src/test/java/com/ibm/icu/dev/test/message2/Mf2FeaturesTest.java 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!

  • icu4j/main/core/src/main/java/com/ibm/icu/message2/NumberFormatterFactory.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@markusicu markusicu marked this pull request as draft September 23, 2024 20:27
break;
}
MFDataModel.Expression expression =
new MFDataModel.VariableExpression(variableRef, null, new ArrayList<>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to change the data model so that selectors are constrained to be Variables rather than Expressions? (If you're trying to avoid a public API change, I understand... although that means it's possible to construct a data model that doesn't correspond to any syntactically correct message.)

@@ -122,6 +122,10 @@
{
"src": "bad {:placeholder @attribute=@foo}"
},
{
"src": "bad {:placeholder @attribute=$foo}",
"ignoreJava": "Unclear why would this be an error?"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants