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

Change addUnit methods to protected and add one more addUnit method #418

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

wujiang1900
Copy link
Contributor

@wujiang1900 wujiang1900 commented Mar 22, 2024

In order for this project to be easily extendable, we can change addUnit methods to protected so that they can be accessed in client applications to add additional units to support the conversions of more unit types.

For the use cases, please see my program: https://github.com/wujiang1900/unit-converter. In particular, please refer to https://github.com/wujiang1900/unit-converter/blob/main/src/main/java/com/challenge/unitconverter/grader/javaximpl/AdditionalUnits.java and see how I'm able to add FAHRENHEIT and RANKINE and support their conversions with the existing CELSIUS unit.

I had to make the changes in this PR and built a new snapshot of indriya to be used in my application to make the above additional unit support working. Therefore please review my PR at your earliest conversion. The code changes below are tiny, but the impact to make indriya flexible and extendable is huge.

Thank you for reviewing!


This change is Reviewable

@keilw keilw self-requested a review March 22, 2024 17:25
Copy link
Member

@keilw keilw left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it sounds reasonable and a little more intuitive to use in subclasses like SI, etc. than the Helper, but can you explain, why the new method's argument is called transformedUnit instead of just unit?
Or change the naming.

@wujiang1900
Copy link
Contributor Author

wujiang1900 commented Mar 22, 2024

The reason that I named the param as transformedUnit is because my use cases as below:
public static final Unit<Temperature> RANKINE = addUnit(new TransformedUnit(FAHRENHEIT, new AddConverter(-459.67)), "Rankine", "R");

I made a new commit to change it to unit as you suggested, so that it conforms the signature of other addUnit methods.

Thanks for reviewing and the comments!

@wujiang1900
Copy link
Contributor Author

keilw

Hi, Werner, I've made the change as you suggested and addressed your comment. Could you review and merge the PR if everything looks ok?

Thank you!

Jiang

@KingRial
Copy link

It would be really nice to have such feature on the library.

I am having the the same need to add new units of measurement to the collection.
In this way also "getUnit" method can be used to access added custom units

@keilw keilw added this to the 2.2.1 milestone Apr 19, 2024
@keilw keilw merged commit 7a33470 into unitsofmeasurement:master Jun 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants