Add numeric_state_expected property to Sensor class #87013

jbouwh · 2023-01-31T10:44:02Z

Proposed change

Add numeric_state_expected helper property to sensor class. The helper will support integrations to validate if numeric value types are supported. An example is the validation of incoming values for a MQTT sensor. For numeric sensors non numeric values are not accepted as state. See also: #87004

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2023-01-31T10:44:08Z

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (sensor) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of sensor can trigger bot actions by commenting:

@home-assistant close Closes the issue.
@home-assistant rename Awesome new title Change the title of the issue.
@home-assistant reopen Reopen the issue.
@home-assistant unassign sensor Removes the current integration label and assignees on the issue, add the integration domain after the command.

frenck

In general, I think this could help. However, I don't think this is a settable property, but more of a helper providing information based on existing information of the entity.

homeassistant/components/sensor/__init__.py

epenet

I think that on line 504, you should replace the set with the new _NON_NUMERIC_DEVICE_CLASSES constant.

And on line 599 you should be able to adjust so that the "is_numeric" check doesn't need to be synchronised.

        if not self.is_numeric:
            return value

homeassistant/components/sensor/__init__.py

MartinHjelmare · 2023-01-31T12:51:43Z

Please extend the PR description and explain how this will be used and where.

epenet

I'm not sure if you saw this comment:

I think that on line 504, you should replace the set with the new _NON_NUMERIC_DEVICE_CLASSES constant.

Also, I'm not convinced about the is_numeric naming of the property.
Not sure about a better name, but maybe numeric_state_expected is better?

MartinHjelmare · 2023-01-31T13:28:16Z

I'm not sure we should make a general helper for this. Normally integrations are in control of the sensor properties already and don't need to check.

jbouwh · 2023-01-31T13:31:52Z

I'm not sure if you saw this comment:

I think that on line 504, you should replace the set with the new _NON_NUMERIC_DEVICE_CLASSES constant.

Also, I'm not convinced about the is_numeric naming of the property. Not sure about a better name, but maybe numeric_state_expected is better?

Have renamed the property.

and...

used the new const at line 504...

epenet · 2023-01-31T13:46:26Z

I'm not sure we should make a general helper for this. Normally integrations are in control of the sensor properties already and don't need to check.

I am not sure how many other integrations need to know if a sensor is expected to be numeric...
But IMO we don't want to have two "sources of truth", even if mqtt is the only integration that needs it.

We already have a check inside the sensor platform, so this is just turning the check into a property that can be used both internally and externally from other integration.

epenet

I think it looks good, but I think @MartinHjelmare or @frenck need to confirm.

homeassistant/components/sensor/__init__.py

frenck · 2023-01-31T15:43:24Z

homeassistant/components/sensor/__init__.py

@@ -246,6 +247,22 @@ def device_class(self) -> SensorDeviceClass | None:
            return self.entity_description.device_class
        return None

+    @final
+    @property
+    def numeric_state_expected(self) -> bool:


This method is partially flawed.

For example, a sensor without state class, without a unit of measurement and without precision and without a device class will return false.

The fact is: We don't know. Could be numeric, could be textual state 🤷

I guess that is its goal, but we should make that more clear in the doc comments.

That happens for sensors that just return a text state.

This would work too:
return not (device_class is None or device_class in NON_NUMERIC_DEVICE_CLASSES)

That results in the same unknown issue.

We have sensors that don't provide enough information to know if it is either number or not (regardless of any logic we can apply). That was my point with my comment above, we should document that.

An sensor can have a '' as a state, but a numeric sensor can not. But I agree it is hard to understand the logic.

That's why I wasn't sure about naming. I didn't like is_numeric because it implied the value had been checked

I proposed numeric_state_expected because if that condition is True, then it HAS to be a number.
If it returns False, then it CAN be a number, but it doesn't HAVE to be.

Adjusted the docstr

Ah the state can be None too, may be better to just set it to Return true if the sensor must be numeric.?
update: adjusted it

homeassistant/components/sensor/__init__.py

Co-authored-by: epenet <[email protected]>

Co-authored-by: Franck Nijhof <[email protected]>

emontnemery

LGTM, thanks @jbouwh 👍

homeassistant/components/sensor/__init__.py

…7013) * Add is_numeric property to Sensor class * Follw up comment * Update homeassistant/components/sensor/__init__.py Co-authored-by: epenet <[email protected]> * Update homeassistant/components/sensor/__init__.py Co-authored-by: epenet <[email protected]> * Tests and corrections * Simplify converion check * Correct custom device class handling * Update homeassistant/components/sensor/__init__.py Co-authored-by: epenet <[email protected]> * rename to numeric_state_expected * Replace with new const * Adjust docstr * Update homeassistant/components/sensor/__init__.py Co-authored-by: Franck Nijhof <[email protected]> * Move to const * Correct logic * Do not use bool * Adjust docstr must be numeric * remote state from docstr * protect numeric_state_expected * Use try_parse_enum for custom class check * Remove redundant type hints --------- Co-authored-by: epenet <[email protected]> Co-authored-by: Franck Nijhof <[email protected]>

home-assistant bot added cla-signed core integration: sensor new-feature Quality Scale: internal labels Jan 31, 2023

frenck reviewed Jan 31, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

epenet reviewed Jan 31, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

epenet reviewed Jan 31, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

epenet reviewed Jan 31, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

epenet reviewed Jan 31, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

epenet reviewed Jan 31, 2023

View reviewed changes

jbouwh marked this pull request as ready for review January 31, 2023 13:25

jbouwh requested a review from a team as a code owner January 31, 2023 13:25

jbouwh changed the title ~~Add is_numeric property to Sensor class~~ Add numeric_state_expected property to Sensor class Jan 31, 2023

epenet approved these changes Jan 31, 2023

View reviewed changes

frenck reviewed Jan 31, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

frenck reviewed Jan 31, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

jbouwh commented Jan 31, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

frenck reviewed Jan 31, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

frenck reviewed Jan 31, 2023

View reviewed changes

jbouwh requested a review from frenck January 31, 2023 19:01

emontnemery reviewed Feb 1, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

epenet reviewed Feb 1, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

epenet reviewed Feb 1, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

jbouwh and others added 19 commits February 1, 2023 14:25

Add is_numeric property to Sensor class

4f73cf9

Follw up comment

3ed1471

Update homeassistant/components/sensor/__init__.py

9c0abf8

Co-authored-by: epenet <[email protected]>

Update homeassistant/components/sensor/__init__.py

caf9401

Co-authored-by: epenet <[email protected]>

Tests and corrections

8b4c5cf

Simplify converion check

16fde59

Correct custom device class handling

fdb5e9d

Update homeassistant/components/sensor/__init__.py

71c2b93

Co-authored-by: epenet <[email protected]>

rename to numeric_state_expected

8c6136d

Replace with new const

15808df

Adjust docstr

fd044ca

Update homeassistant/components/sensor/__init__.py

e23c125

Co-authored-by: Franck Nijhof <[email protected]>

Move to const

cc511e0

Correct logic

36c6c44

Do not use bool

324ad47

Adjust docstr must be numeric

df656ce

remote state from docstr

666c837

protect numeric_state_expected

512175f

Use try_parse_enum for custom class check

20f3ca5

jbouwh force-pushed the sensor-is-numeric branch from 3d9446f to 20f3ca5 Compare February 1, 2023 14:32

Remove redundant type hints

aec9fea

emontnemery approved these changes Feb 1, 2023

View reviewed changes

jbouwh merged commit 2e16b7e into home-assistant:dev Feb 1, 2023

jbouwh deleted the sensor-is-numeric branch February 1, 2023 17:45

MartinHjelmare reviewed Feb 1, 2023

View reviewed changes

homeassistant/components/sensor/__init__.py Show resolved Hide resolved

This was referenced Feb 1, 2023

Address late review on readability sensor code #87111

Merged

Modbus sensors logging warnings for non-numeric value: unavailable (<class 'str'>) on 2023.2.0b4 #86925

Closed

github-actions bot locked and limited conversation to collaborators Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add numeric_state_expected property to Sensor class #87013

Add numeric_state_expected property to Sensor class #87013

jbouwh commented Jan 31, 2023 •

edited

Loading

home-assistant bot commented Jan 31, 2023

frenck left a comment

epenet left a comment

MartinHjelmare commented Jan 31, 2023

epenet left a comment

MartinHjelmare commented Jan 31, 2023

jbouwh commented Jan 31, 2023 •

edited

Loading

epenet commented Jan 31, 2023 •

edited

Loading

epenet left a comment

frenck Jan 31, 2023 •

edited

Loading

jbouwh Jan 31, 2023

frenck Jan 31, 2023 •

edited

Loading

jbouwh Jan 31, 2023 •

edited

Loading

epenet Jan 31, 2023 •

edited

Loading

jbouwh Jan 31, 2023

jbouwh Jan 31, 2023 •

edited

Loading

emontnemery left a comment

Add numeric_state_expected property to Sensor class #87013

Add numeric_state_expected property to Sensor class #87013

Conversation

jbouwh commented Jan 31, 2023 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Jan 31, 2023

frenck left a comment

Choose a reason for hiding this comment

epenet left a comment

Choose a reason for hiding this comment

MartinHjelmare commented Jan 31, 2023

epenet left a comment

Choose a reason for hiding this comment

MartinHjelmare commented Jan 31, 2023

jbouwh commented Jan 31, 2023 • edited Loading

epenet commented Jan 31, 2023 • edited Loading

epenet left a comment

Choose a reason for hiding this comment

frenck Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

jbouwh Jan 31, 2023

Choose a reason for hiding this comment

frenck Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

jbouwh Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

epenet Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

jbouwh Jan 31, 2023

Choose a reason for hiding this comment

jbouwh Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

emontnemery left a comment

Choose a reason for hiding this comment

jbouwh commented Jan 31, 2023 •

edited

Loading

jbouwh commented Jan 31, 2023 •

edited

Loading

epenet commented Jan 31, 2023 •

edited

Loading

frenck Jan 31, 2023 •

edited

Loading

frenck Jan 31, 2023 •

edited

Loading

jbouwh Jan 31, 2023 •

edited

Loading

epenet Jan 31, 2023 •

edited

Loading

jbouwh Jan 31, 2023 •

edited

Loading