Filtered values are no longer rounded if values are not changed/calculated #76164

dgomes · 2022-08-03T21:10:52Z

Breaking change

Values are no longer rounded to the default 2 decimals points for the "Range", "Throttle" and "TimeThrottle" filters, and will keep the source precision

Proposed change

Filtered values are no longer rounded if values are not changed/calculated such as in the case of "Range", "Throttle" and "TimeThrottle" filters

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 Rounding to precision value is applied to all filters #75732
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.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

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

homeassistant/components/filter/sensor.py

veleek

I think the correct approach is just to change the schema.

veleek · 2022-08-09T04:49:12Z

homeassistant/components/filter/sensor.py

@@ -488,7 +488,7 @@ class RangeFilter(Filter, SensorEntity):
    def __init__(
        self,
        entity,
-        precision: int | None = DEFAULT_PRECISION,
+        precision,


Any reason not to keep int | None type definition? Or is it standard to not include it if you're not including a default value?

If I include the type definition mypy complains :(

I think mypy probably complains for a reason...
I started to look into it and I found it very awkward that functions only had partial type hints, and some even had invalid type hints.
I suggest that we fix the type hints first: #86141

veleek · 2022-08-09T04:50:19Z

Failing checks seem unrelated to this change... any way to just re-run them?

balloob · 2022-08-17T02:30:02Z

homeassistant/components/filter/sensor.py

@@ -79,7 +79,7 @@
 ICON = "mdi:chart-line-variant"

 FILTER_SCHEMA = vol.Schema(
-    {vol.Optional(CONF_FILTER_PRECISION, default=DEFAULT_PRECISION): vol.Coerce(int)}
+    {vol.Optional(CONF_FILTER_PRECISION, default=None): vol.Any(None, vol.Coerce(int))}


Definition of DEFAULT_PRECISION can be removed.

I don't understand how removing default precision only applies to 3 filters, while all filter schemas are extending from FILTER_SCHEMA? If we want a different default value, we could keep track of a dictionary mapping filter name => default value, only used when we detect that precision is None.

I'm trying to make this a less of a breaking change as possible, so I don't want to remove DEFAULT_PRECISION (that was my mistake on the refactor after the 1st review)

Also preferred to have the default value next to the class definition instead of a global dict

github-actions · 2022-11-15T16:05:47Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

dgomes · 2022-11-15T16:06:25Z

still valid :(

homeassistant/components/filter/sensor.py

epenet · 2023-01-19T10:08:33Z

I have finished adding typing to filter integration
I suggest that you rebase this PR now.

homeassistant/components/filter/sensor.py

Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>

epenet

The type hint for precision is still wrong on every __init__ method.
I suggest that you change the schema to:

    {vol.Optional(CONF_FILTER_PRECISION): vol.Coerce(int)}

And set the default value on each init method.

def __init__(self, window_size: timedelta, entity: str, precision: int | None = None) -> None:

or

def __init__(self, window_size: timedelta, entity: str, precision: int | None = DEFAULT_PRECISION) -> None:

home-assistant · 2023-03-30T10:22:50Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

dgomes · 2023-03-30T10:32:42Z

Can't set the default value on each init method because there are arguments without default values after precision

epenet · 2023-03-30T10:39:44Z

Can't set the default value on each init method because there are arguments without default values after precision

You can change the order of the arguments, as it is always built using keyword arguments with **_filter:

    filters = [
        FILTERS[_filter.pop(CONF_FILTER_NAME)](entity=entity_id, **_filter)
        for _filter in filter_configs
    ]

epenet · 2023-04-07T13:02:09Z

homeassistant/components/filter/sensor.py

+        if self.filter_precision is not None:
+            filtered.set_precision(self.filter_precision)


Is this change necessary?
Does it not end up being a duplicate of the change on line 384/386?

I believe so, filter_precision starts as None

What I mean is that the first line in set_precision is check that the passed value is not None.
if precision is not None and isinstance(self.state, Number):

So you are first checking that it's not None outside the function, and then checking a second time for the same thing inside the function.

Sorry misunderstood initial comment (fixed)

homeassistant/components/filter/sensor.py

Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>

epenet

LGTM 👍
@balloob are you happy?

balloob · 2023-04-08T03:24:16Z

homeassistant/components/filter/sensor.py

-FILTER_SCHEMA = vol.Schema(
-    {vol.Optional(CONF_FILTER_PRECISION, default=DEFAULT_PRECISION): vol.Coerce(int)}
-)
+FILTER_SCHEMA = vol.Schema({vol.Optional(CONF_FILTER_PRECISION): vol.Coerce(int)})


Just a note for a follow-up PR, since precision is now a user-facing sensor option, this can be completely dropped in a future PR.

probot-home-assistant bot added bugfix integration: filter small-pr PRs with less than 30 lines. labels Aug 3, 2022

homeassistant added the cla-signed label Aug 3, 2022

probot-home-assistant bot added the by-code-owner label Aug 3, 2022

project-bot bot added this to Needs review in Dev Aug 3, 2022

project-bot bot moved this from Needs review to By Code Owner in Dev Aug 3, 2022

frenck added the smash Indicator this PR is close to finish for merging or closing label Aug 4, 2022

veleek reviewed Aug 7, 2022

View reviewed changes

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

veleek reviewed Aug 7, 2022

View reviewed changes

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

veleek reviewed Aug 7, 2022

View reviewed changes

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

Dev automation moved this from By Code Owner to Review in progress Aug 7, 2022

veleek suggested changes Aug 7, 2022

View reviewed changes

veleek reviewed Aug 9, 2022

View reviewed changes

veleek approved these changes Aug 9, 2022

View reviewed changes

dgomes force-pushed the fix_75732 branch from 10bfa25 to 00fc04b Compare August 16, 2022 21:26

balloob reviewed Aug 17, 2022

View reviewed changes

github-actions bot added the stale label Nov 15, 2022

github-actions bot removed the stale label Nov 15, 2022

dgomes mentioned this pull request Jan 17, 2023

Rename precision variable in Filter #86090

Merged

19 tasks

epenet mentioned this pull request Jan 18, 2023

Fix incorrect type hint in Filter #86141

Merged

19 tasks

epenet reviewed Jan 18, 2023

View reviewed changes

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

epenet reviewed Jan 18, 2023

View reviewed changes

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

dgomes and others added 3 commits February 5, 2023 00:23

address 75732

f07f7dd

catchup

46b2322

catchup

104ed33

dgomes force-pushed the fix_75732 branch from 87a4ac6 to 4179780 Compare February 5, 2023 00:27

use default if precision is None

839c0b6

dgomes requested a review from epenet March 28, 2023 20:22

epenet reviewed Mar 29, 2023

View reviewed changes

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

epenet reviewed Mar 29, 2023

View reviewed changes

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

dgomes and others added 2 commits March 30, 2023 11:00

Update homeassistant/components/filter/sensor.py

e1d81d6

Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>

fix type hint

b8b39fa

epenet requested changes Mar 30, 2023

View reviewed changes

home-assistant bot marked this pull request as draft March 30, 2023 10:22

dgomes added 3 commits March 31, 2023 17:21

in progress

fbb6688

Merge remote-tracking branch 'upstream/dev' into fix_75732

13a3d73

refactor

cd1249d

dgomes marked this pull request as ready for review March 31, 2023 17:34

home-assistant bot requested review from epenet and veleek March 31, 2023 17:34

epenet reviewed Apr 7, 2023

View reviewed changes

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

dgomes and others added 3 commits April 7, 2023 14:13

Update homeassistant/components/filter/sensor.py

17daba8

Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>

add *

68df81a

no need to check - review comment

420d06f

epenet approved these changes Apr 7, 2023

View reviewed changes

Dev automation moved this from Review in progress to Reviewer approved Apr 7, 2023

balloob reviewed Apr 8, 2023

View reviewed changes

balloob approved these changes Apr 8, 2023

View reviewed changes

balloob merged commit b1a23c5 into home-assistant:dev Apr 8, 2023
22 checks passed

Dev automation moved this from Reviewer approved to Done Apr 8, 2023

github-actions bot locked and limited conversation to collaborators Apr 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filtered values are no longer rounded if values are not changed/calculated #76164

Filtered values are no longer rounded if values are not changed/calculated #76164

dgomes commented Aug 3, 2022 •

edited

veleek left a comment

veleek Aug 9, 2022

dgomes Aug 9, 2022

epenet Jan 18, 2023

veleek commented Aug 9, 2022

balloob Aug 17, 2022

dgomes Aug 17, 2022 •

edited

github-actions bot commented Nov 15, 2022

dgomes commented Nov 15, 2022

epenet commented Jan 19, 2023 •

edited

epenet left a comment

home-assistant bot commented Mar 30, 2023

dgomes commented Mar 30, 2023 •

edited

epenet commented Mar 30, 2023 •

edited

epenet Apr 7, 2023

dgomes Apr 7, 2023

epenet Apr 7, 2023 •

edited

dgomes Apr 7, 2023

epenet left a comment

balloob Apr 8, 2023

		if self.filter_precision is not None:
		filtered.set_precision(self.filter_precision)

Filtered values are no longer rounded if values are not changed/calculated #76164

Filtered values are no longer rounded if values are not changed/calculated #76164

Conversation

dgomes commented Aug 3, 2022 • edited

Breaking change

Proposed change

Type of change

Additional information

Checklist

veleek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veleek commented Aug 9, 2022

Choose a reason for hiding this comment

dgomes Aug 17, 2022 • edited

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2022

dgomes commented Nov 15, 2022

epenet commented Jan 19, 2023 • edited

epenet left a comment

Choose a reason for hiding this comment

home-assistant bot commented Mar 30, 2023

dgomes commented Mar 30, 2023 • edited

epenet commented Mar 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epenet Apr 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epenet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgomes commented Aug 3, 2022 •

edited

dgomes Aug 17, 2022 •

edited

epenet commented Jan 19, 2023 •

edited

dgomes commented Mar 30, 2023 •

edited

epenet commented Mar 30, 2023 •

edited

epenet Apr 7, 2023 •

edited