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.

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

ICU-21349 Add UnitsConverter.getConversionInfo() #1568

Merged
merged 1 commit into from
Feb 10, 2021
Merged

ICU-21349 Add UnitsConverter.getConversionInfo() #1568

merged 1 commit into from
Feb 10, 2021

Conversation

younies
Copy link
Member

@younies younies commented Feb 8, 2021 •
edited
Loading

Closes: icu-units#24

Checklist

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/intltest/units_test.cpp is different
  • icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java is now changed in the branch
  • icu4j/main/tests/core/src/com/ibm/icu/dev/test/impl/UnitsTest.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

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

~ Your Friendly Jira-GitHub PR Checker Bot

younies added a commit to icu-units/icu that referenced this pull request Feb 9, 2021
@younies
get Conversion Info
44f6d19 
See unicode-org#1568
@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

@younies younies changed the title get Conversion Info ICU-21349 Extract Conversion Info from UnitsConverter Feb 9, 2021
younies added a commit to icu-units/icu that referenced this pull request Feb 9, 2021
@younies
ICU-21349 Extract Conversion Info from UnitsConverter
8446cda 
See unicode-org#1568
@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

@younies younies requested a review from hugovdm February 9, 2021 09:57
hugovdm
hugovdm reviewed Feb 9, 2021
View reviewed changes
Copy link
Contributor

@hugovdm hugovdm left a comment

Choose a reason for hiding this comment

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

(Review at 8446cda)

Looks good - just a couple of comments, with suggestions: if you like the suggestions, you can apply them and squash immediately.

icu4c/source/i18n/units_converter.h
@@ -82,6 +82,12 @@ struct U_I18N_API Factor {
void substituteConstants();
};

struct U_I18N_API ConversionInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

Observation: maybe we will eventually need a sourceOffset and a targetOffset here: it gives us the chance to produce cleaner formulas (e.g. "(5°F − 32) × 5/9 = -15°C").

(Since CLDR does not have separate source and target conversion data and this is just an internal API anyway, this is presently just a design discussion.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Acknowledges :)

icu4c/source/test/intltest/units_test.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/units_test.cpp Show resolved Hide resolved
icu4c/source/test/intltest/units_test.cpp Show resolved Hide resolved
icu4j/main/tests/core/src/com/ibm/icu/dev/test/impl/UnitsTest.java
Comment on lines +315 to +318
new TestData(
"celsius",
"fahrenheit",
9.0 / 5.0, 32, false),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
new TestData(
"celsius",
"fahrenheit",
9.0 / 5.0, 32, false),
new TestData(
"celsius",
"fahrenheit",
9.0 / 5.0, 32, false),
new TestData(
"fahrenheit",
"fahrenheit",
1.0, 0, false),

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added it

icu4c/source/test/intltest/units_test.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/units_test.cpp Outdated Show resolved Hide resolved
@hugovdm hugovdm changed the title ICU-21349 Extract Conversion Info from UnitsConverter ICU-21349 Add UnitsConverter.getConversionInfo(...) Feb 9, 2021
@hugovdm
Copy link
Contributor

hugovdm commented Feb 9, 2021

I've also changed the PR description, since "extract conversion info" made me wonder "to where?"
(I think my suggested PR title would be clearer for readers of the git commit log.)

@younies younies requested a review from hugovdm February 10, 2021 11:53
hugovdm
hugovdm previously approved these changes Feb 10, 2021
View reviewed changes
Copy link
Contributor

@hugovdm hugovdm left a comment

Choose a reason for hiding this comment

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

Looks good!
(at fc94025)

@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

@younies younies changed the title ICU-21349 Add UnitsConverter.getConversionInfo(...) ICU-21349 Add UnitsConverter.getConversionInfo() Feb 10, 2021
@younies younies merged commit 916b9fa into unicode-org:master Feb 10, 2021
@younies younies deleted the conversion-info branch February 10, 2021 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugovdm hugovdm hugovdm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose unit conversion as internal headers
2 participants
@younies @hugovdm