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

QuantityDimension: Fixed divide(Dimension dim) where dim is a product #135

Merged
merged 1 commit into from
Sep 18, 2018

Conversation

DrDaleks
Copy link
Contributor

@DrDaleks DrDaleks commented Sep 17, 2018

That is basically a follow up on #124, where the following tests would fail:

Dimension m2 = QuantityDimension.MASS.pow(2);
Dimension result = QuantityDimension.LENGTH.divide(m2);
assertEquals("[L]/[M]²", result.toString()); // <= returns "[L]/null" instead
Dimension ml = QuantityDimension.MASS.multiply(QuantityDimension.LENGTH);
Dimension result = QuantityDimension.LENGTH.divide(ml);
assertEquals("1/[M]", result.toString()); // <= returns "[L]/null" instead

This is due to divide(Dimension dim) calling multiply(dim.pow(-1)) which has changed behaviour after #129


This change is Reviewable

…uct dimension would return a non-canonical dimension
@DrDaleks
Copy link
Contributor Author

having second (deeper) thoughts. Putting that on hold for now.

@DrDaleks DrDaleks closed this Sep 17, 2018
@DrDaleks
Copy link
Contributor Author

So here's the thing. #129 changed the handling of powered units in a way that any unit (and therefore pseudounit) calling a pow(int n) encapsulates the original unit, thereby a) fixing formatting issues in SimpleUnitFormat (see #124), but b) preventing the canonisation process.

The calls to ProductUnit.ofQuotient(a, b) circumvent the issue, but the question is: is this the right path, or should we revert #129 and have a profound refactoring of SimpleUnitFormat instead (which I don't see as simple in it's current state).

I'd rather be in favor of pushing this further (for now) and returning to this issue (if it is one) at a later time

@DrDaleks DrDaleks reopened this Sep 18, 2018
@keilw
Copy link
Member

keilw commented Sep 18, 2018

Are the tests working now?

@DrDaleks
Copy link
Contributor Author

Yes, I've added 2 unit tests for the issue (both passing).

@DrDaleks
Copy link
Contributor Author

I did however found other (similar) issues related to unit.pow(-1) outside of the current (dimension) scope. Will probably make another PR soon for those cases.

@keilw keilw merged commit ceea0c2 into unitsofmeasurement:master Sep 18, 2018
@keilw
Copy link
Member

keilw commented Sep 18, 2018

@DrDaleks Thanks, also for considering to join the JSR. It seems, you are not a JCP Member yet. Are you employed by your company (it is not a JCP member either) or work as a freelancer/contractor? Permanent employees usually need their employer to sign the JSPA and agreements to join a JSR as Expert, if that was not easy or possible, please join as Associate Member. I approved it but depending on which member category you get, it could be necessary to re-apply as Contributor (unless the PMO could tweak that but I'm not sure)

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