[openhab.core] DecimalType-ctor with Number argument

[kippAndMost]

The expectation is, that when a DecimalType was constructed with a
float value, the precision of its doubleValue(), floatValue(),
toBigDecimal() and toString() is preserved. But there are float
values like 4.2f or 37.1f that cannot directly converted to double
without precision loss.

This commit replaces all the numerical constructors of DecimalType
with a single constructor with a Number argument, so that all float
values can be used without precision loss.

NOTE: There is some special handling needed for QuantityType and
HSBType because these types have a special none conventional toString
implementation.

[cweitkamp]

Thanks for your contribution.

Wouldn't it make more sense to refactor the class to have a generic constructor passing a Number as parameter instead of one constructor for each primitive type?

[kippAndMost]

Yes, I am with you. That was actually my first thought. That would also be consistent with the implementation of QuantityType. Then one would discard all the other constructors which use a number in whatever form. However, I then decided to do the minimum impact possible.

But I would of course welcome the consistent solution with the Number-ctor. What would you recommend?

[cweitkamp]

I would like to go for the Number c-tor.

We will have feature freezer on next Monday ( 13th or December ) thus I would like to reschedule this change to a later time to make sure we do not introduce unwanted side effects. I hope that is okay for you.

[kippAndMost]

Yes of course, its OK for me.

I replaced my previous commit with an appropriate one.

[cweitkamp]

Thanks for the changes.

[cweitkamp]

@kippAndMost Are you still around to finalize this PR?

[kippAndMost]

Hi cweitkamp, sorry, I have some work overload at the moment. I will have a look at as soon as possible.

[cweitkamp]

LGTM. Thanks.

[wborn]

I think this PR has broken the add-ons build, can you have a look at it @kippAndMost?

See: https://ci.openhab.org/job/openHAB-Addons/638/

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:testCompile (default-testCompile) on project org.openhab.binding.upnpcontrol: Compilation failure: Compilation failure:
[ERROR] /home/jenkins/jenkins-agent1/workspace/openHAB-Addons/bundles/org.openhab.binding.upnpcontrol/src/test/java/org/openhab/binding/upnpcontrol/internal/handler/UpnpRendererHandlerTest.java:[925,64] Null type mismatch (type annotations): required 'java.lang.@NonNull Number' but this expression has type 'java.lang.@Nullable Integer'
[ERROR] /home/jenkins/jenkins-agent1/workspace/openHAB-Addons/bundles/org.openhab.binding.upnpcontrol/src/test/java/org/openhab/binding/upnpcontrol/internal/handler/UpnpRendererHandlerTest.java:[926,28] Null type mismatch (type annotations): required 'java.lang.@NonNull Number' but this expression has type 'java.lang.@Nullable Integer'

[jimtng]

This broke Fronius binding:

java.lang.NoSuchMethodError: 'void org.openhab.core.library.types.DecimalType.<init>(double)'

A fix for Fronius binding has been submitted in a PR.

A quick search shows this addon file using the same cast to (double) openhab-addons/bundles/org.openhab.binding.ihc/src/main/java/org/openhab/binding/ihc/internal/handler/IhcHandler.java

[jimtng]

and openweathermap

java.lang.NoSuchMethodError: 'void org.openhab.core.library.types.DecimalType.<init>(double)'
        at org.openhab.binding.openweathermap.internal.handler.AbstractOpenWeatherMapHandler.getDecimalTypeState(AbstractOpenWeatherMapHandler.java:185) ~[?:?]
        at org.openhab.binding.openweathermap.internal.handler.OpenWeatherMapOneCallHandler.updateCurrentChannel(OpenWeatherMapOneCallHandler.java:347) ~[?:?]

[J-N-K]

Looks like this is heavily breaking (binary) API compatibility.

[jimtng]

Can't it keep the old constructors as well?

[J-N-K]

Did you check if just re-compiling also fixes the problem? I believe this is only incompatible in binary code, not at source level.

[cweitkamp]

@J-N-K is right.

We first should fix the build and release a new snapshot before starting to look into every binding. I hope the only think which I missed in this PR is the DecimalType(BigDecimal) constructor which in general is superfluous a as BigDecimal is extends Number.

[J-N-K]

@cweitkamp But maybe keeping the old constructors is also a good idea to maintain binary compatibility. They can all call a single new one. Maybe mark them as @Deprecated.

Breaking binary compatibility here would require all add-ons from the Community-Marketplace (and JSON 3rd Party) to be either compatible with 3.2 or 3.3 (there is no versioning). While this can be easily managed for JSON 3rd Party by just providing a different version in another JSON-file, the community marketplace currently can't handle that and would require bigger refactoring of the add-on service and probably an additional flag in Discourse.

[cweitkamp]

I submitted a PR to fix the build openhab/openhab-addons#12374

You got a point. That is a strong argument.

@openhab/core-maintainers Wdyt? Should we reintroduce the removed constructors.

[J-N-K]

I think this should be decided before M2.

[Hilbrand]

This will actually break compatibility with 3.2 for any binding that is compiled against that 3.3 where DecimalType is used in combination with an object instead primitive (for example Integer) because that type will compile against Number constructor that doesn't exist in 3.2. Meaning anyone updating the binding compiling it with 3.3 and puts it in the market place will have this issue. 😞