unitsofmeasurement / uom-systems

Units of Measurement Systems
http://www.uom.systems
Other
36 stars 17 forks source link

I think the ) was in the wrong place here. #163

Closed maxvonhippel closed 4 years ago

maxvonhippel commented 4 years ago

Comparing this line to similar ones, I think the ) is in the wrong place.

In the relatively likely event that I'm wrong, it would be awesome if you could explain why, so I can better understand the code!

Thanks!


This change is Reviewable

keilw commented 4 years ago

Thanks for the suggestion, which one did you see in a different way? I think it is correct here as it was because otherwise the type is lost in the internal representation.

maxvonhippel commented 4 years ago

@keilw for example on line 241:

public static final Unit<Speed> MILE_PER_HOUR = addUnit(MILE.divide(HOUR).asType(Speed.class), "Mile per hour", "mph");

Here the .asType(...) happens inside the addUnit(...). Is that incorrect? Thank you!

andi-huber commented 4 years ago

The addUnit(...) method just acts as an identity function with side-effects.

So both variants discussed here are correct. The unit's generic type information is lost at runtime anyway: Generic Type Erasure.

You can even omit the asType(...) cast here without breaking anything. We really only use it to remove compile time warnings.

keilw commented 4 years ago

It would not compile without any call to asType or an explicit cast, at least on the outside, so to the outside method signature it seems fine, but we should do it a little more consistently.

maxvonhippel commented 4 years ago

Cool! Thank you for the explanation, I appreciate it. I recommend closing this issue, since my PR does not improve anything.