ICU-23265 Fixed parseMeasureUnitOption() so that it correctly handles…

[richgillam]

… measurement unit aliases.

The idea here is to fix it so that NumberFormat::forSkeleton() doesn't return a syntax error when the skeleton includes a measurement unit that has been renamed, such as concentr-permillion. The idea is to maintain backward compatibility.

Checklist

• Required: Issue filed: ICU-23265
• Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable
• Approver: Feel free to merge on my behalf

[sffc]

Did you consider changing the impl of MeasureUnit::validateAndGet ?

[richgillam]

Did you consider changing the impl of MeasureUnit::validateAndGet ?

Yes, but not for very long. I was back-porting a fix I'd already made in Apple ICU, and validateAndGet() didn't exist at that time. I opted to keep my original fix as intact as I could, rather than trying to re-think it. I don't have a lot of time, and I'm uncomfortable in this code base. I was kind of hoping that if the fix wasn't good enough, somebody who knew the code better than I do (such as you or Younies) would be able to give me more specific instructions on what a better fix would look like. I'll take another look and see I can see an easy way to incorporate this change into that function, but if it's not fairly simple and straightforward, I'd like permission to leave it this way and let somebody else clean it up later.

[richgillam]

Did you consider changing the impl of MeasureUnit::validateAndGet ?

Okay, I went back and took another look, and it actually wasn't that bad to move the alias handling code into validateAndGet(), so I did it and I hope you'll take another look.

This wound up being a Java-only fix because there is no Java version of MeasureUnit::validateAndGet() as far as I could tell, and the equivalent method-- MeasureUnit.getUnit()-- didn't look like a safe place to insert the alias-handling logic.

I think this commit should also fix the mvn spotless errors I was getting with my initial commit...

[sffc]

This is better, thanks. To keep C++ and Java aligned, I think I'd rather see the Java change pulled into a new function MeasureUnit.validateAndGet that looks like the C++ function.

[richgillam]

This is better, thanks. To keep C++ and Java aligned, I think I'd rather see the Java change pulled into a new function MeasureUnit.validateAndGet that looks like the C++ function.

Done.

[jira-pull-request-webhook]

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

~ Your Friendly Jira-GitHub PR Checker Bot

[sffc]

Observation: you don't have if (!result.getType().equals(type)) in the C++ code

[richgillam]

Yes, I do. The check for typeIdx >= 0 on line 2774 is doing that.

[richgillam]

Actually, you're right. I don't have that exact check. I'll add it...

[richgillam]

In case anybody's wondering, mvn spotless:check fails miserably for me, with a whole bunch of NoClassDefFoundErrors, most complaining about JCTree$JCAnyPattern. I'm guessing a versioning issue of some kind (or maybe a JDK issue?), but that's why there are all these extra commits to fix Java style-checker issues.

[jira-pull-request-webhook]

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

~ Your Friendly Jira-GitHub PR Checker Bot