Upgrade Units of Measurement dependencies #10583
Conversation
wborn
requested review from
cpmeister,
cweitkamp,
gerrieg,
Hilbrand,
hmerk,
kaikreuzer,
marcelrv,
paulianttila,
ssalonen,
svilenvul and
a team
as code owners
* Fix code/tests for upgrade * Resolve runbundles * Update Checkstyle ruleset for changed packages Signed-off-by: Wouter Born <[email protected]>
wborn
added
rebuild
|
This PR needs to be merged so other PR builds succeed again @openhab/add-ons-maintainers. ✔️ |
| ((QuantityType<?>) actualStateUpdateTowardsItem).getUnit()); | ||
| assertEquals(((QuantityType<?>) expectedStateUpdateTowardsItem).toBigDecimal(), | ||
| ((QuantityType<?>) actualStateUpdateTowardsItem).toBigDecimal()); | ||
| assertEquals(expectedStateUpdateTowardsItem, actualStateUpdateTowardsItem); |
There was a problem hiding this comment.
Interesting to see the complicated equals check is no longer needed. I suppose it was remnant from time (during development) when I tried to make the profile support gain/offset for any QuantityType. This caused unexpected temperature unit errors like indicated in the comment above.
If the tests pass, it should be fine.
One change suggestion, can you remove now unnecessary conditional and the obsolete comment on "workaround"
There was a problem hiding this comment.
unnecessary conditional
Which one do you think is unnecessary?
There was a problem hiding this comment.
@wborn it now says
// Workaround for errors like "java.lang.UnsupportedOperationException: °C is non-linear, cannot convert"
if (expectedStateUpdateTowardsItem instanceof QuantityType<?>) {
assertTrue(actualStateUpdateTowardsItem instanceof QuantityType<?>);
assertEquals(expectedStateUpdateTowardsItem, actualStateUpdateTowardsItem);
} else {
assertEquals(expectedStateUpdateTowardsItem, actualStateUpdateTowardsItem);
}
As you can see, both branches of conditional are effectively the same. Also, the comment does not apply anymore, you in fact were able to remove the "workaround"
There was a problem hiding this comment.
OK now I understand, I've created #10670 to clean it up. 👍
| private <QU extends Quantity<QU>> QuantityType<QU> applyGainTowardsItem(QuantityType<Dimensionless> qtState, | ||
| QuantityType<QU> gainDelta) { | ||
| return (QuantityType<QU>) qtState.multiply(gainDelta); | ||
| return new QuantityType<>(qtState.toBigDecimal().multiply(gainDelta.toBigDecimal()), gainDelta.getUnit()); |
There was a problem hiding this comment.
Looks to be correct, tests should reveal any issues.
I am not fond of bypassing quantitytype-arithmetic since we lose many of the correctness guarantees from the uom lib. Having said this , in this particular case the change is isolated and allows to remove the unchecked cast.
| assertEquals("14.0 W", b.gridPowerSupply.toString(), "Grid Supply"); | ||
| assertEquals("0.0 W", b.gridPowerConsumpition.toString(), "Grid Consumption"); | ||
| assertEquals("303.0 W", b.batteryPowerSupply.toString(), "Battery Supply"); | ||
| assertEquals("-330 W", b.pvPowerSupply.toString(), "PV Supply"); |
There was a problem hiding this comment.
I am not maintainer of modbus.e3dc but this seems like a harmless side effect of library change. I would not expect this to have negative side effects to non-test code.
There was a problem hiding this comment.
Yes toString values aren't the most future proof values to use in such comparisons.
They are easy to read and understand however. So that's why I opted for just updating them.
* Fix code/tests for upgrade * Resolve runbundles * Update Checkstyle ruleset for changed packages Signed-off-by: Wouter Born <[email protected]>
* Fix code/tests for upgrade * Resolve runbundles * Update Checkstyle ruleset for changed packages Signed-off-by: Wouter Born <[email protected]>
* Fix code/tests for upgrade * Resolve runbundles * Update Checkstyle ruleset for changed packages Signed-off-by: Wouter Born <[email protected]>
* Fix code/tests for upgrade * Resolve runbundles * Update Checkstyle ruleset for changed packages Signed-off-by: Wouter Born <[email protected]>
Depends on openhab/openhab-core#2319