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

[automation] Log warning for equals condition for DecimalTypes and QuantityTypes #2653

Merged
merged 3 commits into from
Jan 15, 2022

Conversation

cweitkamp
Copy link
Contributor

@cweitkamp cweitkamp commented Dec 29, 2021

  • Log warning for equals condition for DecimalTypes and QuantityTypes

Closes #2624

Signed-off-by: Christoph Weitkamp github@christophweitkamp.de

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp cweitkamp added bug An unexpected problem or unintended behavior of the Core automation labels Dec 29, 2021
@cweitkamp cweitkamp requested a review from a team as a code owner December 29, 2021 18:38
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Isn't the real issue about #2624 that TypeParser.parseState(item.getAcceptedDataTypes(), state) returns a DecimalType instead of a QuantityType? Shouldn't item.getAcceptedDataTypes() return QuantityType before DecimalType in case the number item has a dimension?

@@ -65,9 +65,9 @@ public ParameterSet(String comparisonState, State itemState, boolean expectedRes
{ new ParameterSet("5", new DecimalType(23), false) }, //
{ new ParameterSet("5", new DecimalType(5), true) }, //
{ new ParameterSet("5 °C", new DecimalType(23), false) }, //
{ new ParameterSet("5 °C", new DecimalType(5), false) }, //
{ new ParameterSet("5 °C", new DecimalType(5), true) }, //
Copy link
Member

Choose a reason for hiding this comment

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

I would claim that this change is not correct.
The unit test was just right before: 5 is NOT the same as 5 °C as it is lacking any unit and could thus well also be 5 °F.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping this in mind we should adopt the remaining comparisons too and add a conversion to a base unit before. I will have a look.

@cweitkamp
Copy link
Contributor Author

From my pov the roit cause of #2624 is that the user did not configuration a comparison value including a unit. And additionally the framework did not inform him about his mistake.

IIRC we cannot change the order of accepted data types for Number items because every plain number without a unit will parsed by the QuantityType with unit ONE.

I will revert my changes partially to just log a warning if the item state is a QuantityType but comparison value does not have a unit.

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp cweitkamp changed the title [automation] Allow equals comparison for DecimalTypes and QuantityTypes [automation] Log warning for equals condition for DecimalTypes and QuantityTypes Dec 30, 2021
State itemState = item.getState();
if (itemState instanceof QuantityType && compareState instanceof DecimalType) {
logger.warn(
"Received a QuantityType state '{}' with unit for item {}, but the condition is defined as a plain number without unit ({}), please consider adding a unit to the condition.",
Copy link
Member

Choose a reason for hiding this comment

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

"please consider adding" sounds as if it is optional and will work nonetheless without.
Shouldn't we maybe use some stronger formulation to make it clear that the current configuration is invalid and the comparison is simply not done (always returns 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 reformulated the warning to be more precise.

Additionally I added a check to deal with dimensionless QuantityTypes without unit (internally AbstractUnit.ONE) because for them it is not possible to define a unit in the compare condition as the unit symbol is an empty string.

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp cweitkamp added enhancement An enhancement or new feature of the Core and removed bug An unexpected problem or unintended behavior of the Core labels Jan 1, 2022
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/bug-or-why-rule-working-with-temperature/130944/6

@wborn wborn requested a review from kaikreuzer January 15, 2022 12:27
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thank you!

@kaikreuzer kaikreuzer merged commit 0709933 into openhab:main Jan 15, 2022
@kaikreuzer kaikreuzer added this to the 3.3 milestone Jan 15, 2022
@cweitkamp cweitkamp deleted the bugfix-2624 branch January 15, 2022 15:10
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
…antityTypes (openhab#2653)

* Allow equals comparison for DecimalTypes and QuantityTypes

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
GitOrigin-RevId: 0709933
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The "equals" condition does not work for the items storing dimensionless number values
3 participants