Skip to content
This repository has been archived by the owner on May 26, 2021. It is now read-only.

Enhancement: AbstractUnit's final methods now guarantee to return AbstractUnit<> instead of just Unit<> #176

Merged
merged 3 commits into from
Aug 13, 2017

Conversation

Pr0methean
Copy link
Contributor

@Pr0methean Pr0methean commented Aug 12, 2017

Narrows the return type of final methods in AbstractUnit from Unit<> to AbstractUnit<>. The only behavioural change is that if a third-party Unit<> that doesn't extend AbstractUnit<> is passed to one of these methods for transformation, it will be wrapped in a ProductUnit even when that wasn't previously necessary. This may cause some tests that rely on reference equality to fail; but in a near-future PR, I'll fix this by refining the implementation of ProductUnit.equals().

@Pr0methean Pr0methean closed this Aug 12, 2017
@Pr0methean Pr0methean reopened this Aug 12, 2017
@Pr0methean Pr0methean changed the title Enhancement: AbstractUnit's final methods now guarantee to return AbstractUnit<?> Enhancement: AbstractUnit's final methods now guarantee to return AbstractUnit<> instead of just Unit<> Aug 13, 2017
@keilw keilw merged commit c2ae9d4 into unitsofmeasurement:master Aug 13, 2017
@keilw
Copy link
Member

keilw commented Aug 13, 2017

Please also consider porting some of those PRs to https://github.com/unitsofmeasurement/indriya. It is currently in "stealth mode" but supposed to be a unified RI of an upcoming new Units JSR.

@keilw
Copy link
Member

keilw commented Aug 16, 2017

@Pr0methean What is the main benefit or improvement of returning AbstractUnit instead of Unit by most methods in the AbstractUnit class? Not sure about the static parse() method btw. I saw it also returns AbstractUnit. Before applying to Indriya which will be a future RI of the next API/JSR version, we should understand what was won from this change. A possible goal of a new API could be to allow mixing implementations in the same module or class path, see unitsofmeasurement/unit-api#38.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants