unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
115 stars 40 forks source link

Number conversion issue using indriya with Scala language #313

Open ckittl opened 3 years ago

ckittl commented 3 years ago

Hey guys!

I recently experienced some issues using your lib together with the Scala language und therefore would like you to raise some awareness for this point.

The root of the problem is, that scala.math.BigDecimal extends java.lang.Number, therefore an instance of it is accepted in Quantities#getQuantity(Number, Value). However, later in the processing, "only" java.math.BigDecimal is covered:

https://github.com/unitsofmeasurement/indriya/blob/233f00ff898c34f1e651b47dc5ee81560b135800/src/main/java/tech/units/indriya/function/DefaultNumberSystem.java#L98-L135

Therefore

val a = BigDecimal("5.2")
Quantities.getQuantity(a, Units.METRE)

will lead to IllegalArgumentException.

Most likely this issue is out of scope for your lib, but I just wanted to document this issue for others, that might experience that problem as well. For a better depiction I placed some examples in this example repo.

If you can provide you with any further information, please just let me know.

keilw commented 3 years ago

So essentially it seems that Scala has its own type scala.math.BigDecimal extending java.lang.Number and it won't map directly to java.math.BigDecimal? We need to have a look, it's probably possible to use the class name without directly importing anything, somewhat similar to what we also do in the API to resolve various DI mechanisms like javax.inject or jakarta.inject (from the upcoming MR1 on)

Another alternative we also hinted with #254 would be to create your own implementation of NumberSystem.

ckittl commented 3 years ago

Thanks a lot for your fast hints on how to come around this issue!

I really like the idea of providing a different implementation of NumberSystem. Therefore, I have a question regarding your current implementation:

https://github.com/unitsofmeasurement/indriya/blob/233f00ff898c34f1e651b47dc5ee81560b135800/src/main/java/tech/units/indriya/function/Calculus.java#L88-L93

Is it intentionally, that you don't use the ServiceLoader here to allow for custom implementations ("If there is one specific other service than default, use that. In any other cases use default.")? Or do I just misunderstand?

keilw commented 3 years ago

That's a good question, hope @andi-huber can also say something about it, but I believe the default existed before the SPI definition, so it might need some brushing-up and streamlining.

andi-huber commented 3 years ago

Hi there, I believe that is not my code and indeed it looks broken.

andi-huber commented 3 years ago

Sorry, or I might just have missed the concept, because this should work ...

Calculus.setCurrentNumberSystem(Calculus.getNumberSystem("org.my.NumberSystem"));

... where the ServiceLoader is used to lookup an implementation by name of org.my.NumberSystem

andi-huber commented 3 years ago

To answer the question, yes its intentional, so when multiple NumberSystems are on the class-path (visible to the ServiceLoader), its up to the library consumer to pick the one meant to be used.

keilw commented 3 years ago

I think it's correct, the quote was from the method currentNumberSystem() where picking an explicit default is a fallback should the current one be null. getNumberSystem("org.my.NumberSystem") tries to resolve a system from the ServiceLoader.

ckittl commented 3 years ago

Thanks a lot for your comments!

I had in mind to simply place a tech.units.indriya.NumberSystem-file in META-INF/services and "hope" that this brings only one distinct other NumberSystem-Implementation besides the default one (thus not asking the user to add anything in code). But as your way / additions is / are more explicit, it is more favourably to follow this path.

keilw commented 3 years ago

That's already there. I am not sure, if multiple entries would be possible but Indriya only defines one anyway. Our extension modules like UCUM do that, e.g. their own ServiceProvider. So far Indriya is multi-release-jar capable, but not as modular as let's say the JSR 354 RI Moneta. Where different modules define their ExchangeRateProvider for ECB or IMF. One could imagine breaking Indriya down into a few modules at some point, and it would allow to have one default NumberSystem like it is right now and another module that could use Apache Commons Numbers once that is Final.

ckittl commented 3 years ago

I think we just talked about different things. If you stay on the level of your project it is doable and nearly there, as you provide some kind of "coordination" in this way, as you only provide one number system.

What I thought about first is, me as a user placing a tech.units.indriya.NumberSystem-file in META-INF/services and all the rest happens without doing anything else. This is not a good idea, as you as developers of the lib can only hardly guess what I will place there and would have the need to do some assumptions.

Just wanted to clarify it - at least I had some moment of confusion. 😃