icu
icu copied to clipboard
ICU-22746 Refactor some MF2 tests to be data-driven
This change moves all test strings out of test/intltest/messageformat2test.cpp and into JSON files, which are parsed/run by code in a new file, test/intltest/messageformat2test_read_json.cpp . It also removes the file test/intltest/messageformat2test_fromjson.cpp , which contained tests that are now stored in JSON files.
Some of the tests in other files will need to be converted; this is an incremental change.
A new vendored library, https://github.com/nlohmann/json/ , is added as a dependency. This accounts for most of the "files changed".
Checklist
- [x] Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22746
- [x] Required: The PR title must be prefixed with a JIRA Issue number.
- [x] Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
- [x] Required: Each commit message must be prefixed with a JIRA Issue number.
- [ ] Issue accepted (done by Technical Committee after discussion)
- [x] Tests included, if applicable
- [x] API docs and/or User Guide docs changed or added, if applicable
cc @echeran -- I didn't fix the copyright scan errors since you mentioned you would help with legal signoff re: licensing for the nlohmann/json library.
cc @srl295 @roubert -- I tried to follow what you suggested with vendoring the library; let me know if this is what you had in mind. I blatantly copy-pasted the pull-from-upstream.sh and UPDATING.md files from the vendor/double-conversion
directory, but if you think it's worth generalizing the instructions and shell script, let me know.
good work and a couple of comments
- i see two copies of ujson.h and probably only need one
- does the entire repo need to be vendored since only one file is used? perhaps only the .h file and the license?
- it might be good to have a file with the latest hash and version or such (in the vendor directory)
good work and a couple of comments
- i see two copies of ujson.h and probably only need one
Thanks for spotting that, fixed.
- does the entire repo need to be vendored since only one file is used? perhaps only the .h file and the license?
Probably not, no. Changed this in 24de7c4 and removed the extra files.
- it might be good to have a file with the latest hash and version or such (in the vendor directory)
Do you mean a manually updated file, or changing the pull-from-upstream.sh script to update the file when it's run?
- [ ] also this PR needs to update the top level
LICENSE
file to include a copy of LICENSE.MIT, see other examples - [ ] cpyscan tool can exclude .json files and the vendor/ directory.
I double checked with Google's legal counsel for open source, who gave approval continent on guidelines that already match ICU's current processes for vendoring 3rd party sources:
It will be fine to vendor MIT-licensed code into ICU. ... The LICENSE file that lives alongside the nlohmann/json code in the
/vendor
subdirectory of ICU needs to inherit the specific MIT license file at https://github.com/nlohmann/json/blob/develop/LICENSE.MIT with its upstream copyright notice intact.
- should have some mention of checking that the license doesn't change
I think your suggested change, which I incorporated, addresses this?
OK, I think I've incorporated all the licensing changes you suggested, @echeran and @srl295 -- can you look again? Thanks!
@catamorphism all LGTM!
- probably want to squash it per ICU prefs - https://us-central1-dev-infra-273822.cloudfunctions.net/unicode-github-bot/info/unicode-org/icu/2980 (the 'Details' under the single-commit check) will do it for you
- @echeran are you driving the ticket and the merge?
Hooray! The files in the branch are the same across the force-push. 😃
~ Your Friendly Jira-GitHub PR Checker Bot
Thanks! I've squashed.