Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix QuantityType#eql? raising exception on incompatible QuantityType unit #400

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Feb 16, 2025

There was a bug in our spec which core was happily allowing:

(1 | "1/W").compare_to(1 | "W") used to return 0.

openHAB 5.0 now throws an exception when comparing invertible unit.
Relevant PR

@jimtng jimtng force-pushed the divide-by-quantity branch from 52a5423 to 12cac5c Compare February 16, 2025 13:03
@jimtng jimtng added the bug Something isn't working label Feb 16, 2025
@jimtng jimtng requested a review from ccutrer February 16, 2025 13:50
@jimtng jimtng marked this pull request as ready for review February 16, 2025 13:50
@jimtng jimtng force-pushed the divide-by-quantity branch 3 times, most recently from 508516f to 8b092be Compare February 17, 2025 14:43
@jimtng
Copy link
Contributor Author

jimtng commented Feb 17, 2025

I've restored eql? and added a unit compatibility check to avoid the exception from core's compare_to

@jimtng jimtng force-pushed the divide-by-quantity branch 2 times, most recently from eda0315 to 7b25386 Compare February 17, 2025 14:56
@jimtng jimtng changed the title Fix NumericType#eql? raising exception on incompatible QuantityType unit Fix QuantityType#eql? raising exception on incompatible QuantityType unit Feb 17, 2025
@jimtng jimtng force-pushed the divide-by-quantity branch 3 times, most recently from eae3c33 to 90ec40e Compare February 17, 2025 15:12
@@ -483,7 +483,7 @@
expect(2 * w).to eql 10 | "W"
expect((2 * kw).to_i).to eq 10_000
expect(2 * w == 10).to be true
expect(5 / w).to eql 1 | "W"
expect(5 / w).to eql 1 | "/W"
expect((2 * w / 2)).to eql w
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add a not_to eql case for when it's false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a good not_to eql check to add, so I added a check against kw. Do you have a better idea?

…unit

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng jimtng force-pushed the divide-by-quantity branch from 90ec40e to a6c07fe Compare February 17, 2025 23:31
Copy link
Contributor

@ccutrer ccutrer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's exactly what I had in mind

@ccutrer ccutrer merged commit 71f02b6 into openhab:main Feb 18, 2025
26 checks passed
@jimtng jimtng deleted the divide-by-quantity branch February 19, 2025 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants