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

CLDR-11155 Separate Volume_Metric and Volume_Other #3526

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

btangmu
Copy link
Member

@btangmu btangmu commented Feb 26, 2024

-New subroutines makePathHeader and getVolumePageId

-Change example in Dashboard.java from Volume to Area

-Comments

CLDR-11155

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

@btangmu btangmu self-assigned this Feb 26, 2024
@btangmu
Copy link
Member Author

btangmu commented Feb 26, 2024

This works except that some paths that should be in Volume_Metric are in Volume_Other, such as:

//ldml/units/unitLength[@type="narrow"]/unit[@type="volume-cubic-kilometer"]/displayName

I don't know why, except that unitSystemCollection is empty in some such cases...

@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

@@ -766,6 +752,58 @@ public PathHeader fromPath(final String path, List<String> failures) {
}
}

private PathHeader makePathHeader(RawData data, String path, String alt) {
// Caution: each call to PathHeader.Factory.fix changes the value of
// PathHeader.Factory.order
Copy link
Member Author

Choose a reason for hiding this comment

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

One improvement (encapsulation for clarity) might be for fix() to return, or fill in, a small object containing a string (name/code), an "order", and whatever else it sets (suborder?) -- instead of directly modifying elements of Factory

Also, the PathHeader constructor has 9 parameters! Anything more than 4 is considered smelly code, so the constructor should be revised along with this method

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/util/PathHeader.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@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

@macchiati
Copy link
Member

macchiati commented Feb 26, 2024 via email

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/util/PathHeader.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

-New subroutines makePathHeader and getVolumePageId

-Change example in Dashboard.java from Volume to Area

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

@btangmu btangmu marked this pull request as ready for review February 27, 2024 14:32
@btangmu
Copy link
Member Author

btangmu commented Feb 27, 2024

@macchiati with getSystemsEnum it appears to work perfectly

final String longUnitId =
XPathParts.getFrozenInstance(path).findAttributeValue("unit", "type");
if (longUnitId == null) {
throw new RuntimeException("Missing unit in path " + path);
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't throw very generic exceptions, because it makes debugging trickier. If you touch this again, it would be better to have a more specific exception.

SectionId newSectionId = SectionId.forString(fix(data.section, 0));
String pageIdName = fix(data.page, 0);
PageId newPageId;
if ("Volume".equals(pageIdName)) {
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this should be done inside of the fix.

Copy link
Member

@macchiati macchiati left a comment

Choose a reason for hiding this comment

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

Approved (some minor improvements could be made, but not blockers)

@btangmu btangmu merged commit 9c8175a into unicode-org:main Feb 28, 2024
10 checks passed
@btangmu btangmu deleted the t11155_e branch February 28, 2024 14:03
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