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

Add last_reset for Sense trend sensors #63490

Merged
merged 5 commits into from
Jan 20, 2022
Merged

Conversation

kbickar
Copy link
Contributor

@kbickar kbickar commented Jan 5, 2022

Proposed change

Adds the last_reset attribute for trend sensors. This prevents them from showing a large spike at the start of the day

A small update was needed for the sense api library to allow the date to be passed to fix a problem with the last_reset not being set correctly if the timezone of the HA machine didn't match.
https://github.com/scottbonline/sense/releases/tag/0.9.4
https://github.com/scottbonline/sense/releases/tag/0.9.5
https://github.com/scottbonline/sense/releases/tag/0.9.6

scottbonline/sense@0.9.3...0.9.6

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

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:

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:

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Jan 5, 2022
@epenet
Copy link
Contributor

epenet commented Jan 7, 2022

Please add a link to the changelog in the description

homeassistant/components/sense/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/sense/__init__.py Outdated Show resolved Hide resolved
@kbickar
Copy link
Contributor Author

kbickar commented Jan 19, 2022

@emontnemery Made some updates to the library - it seems the API will return the start of the trend data if you pass it what it'd expecting. Much less difference in HA now

@bdraco
Copy link
Member

bdraco commented Jan 19, 2022

Thats a lot cleaner. I'll install this on my production systems and report back in a few days

@emontnemery
Copy link
Contributor

This looks perfectly fine from a Home Assistant POV now.

A comment on the change in the library though, please don't do bare excepts, do like this instead:

        try:
             start_iso = self._trend_data[scale]['start'].replace('Z', '+00:00')
             return datetime.strptime(start_iso, '%Y-%m-%dT%H:%M:%S.%f%z')
-       except:
+       except Exception:
             return None

@frenck frenck removed the smash Indicator this PR is close to finish for merging or closing label Jan 19, 2022
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

So far so good. Waiting for the day to roll over to confirm

@bdraco bdraco self-requested a review January 20, 2022 04:06
@bdraco
Copy link
Member

bdraco commented Jan 20, 2022

Looking good. Waiting for the sun to come up and the solar to kick in for a full test cycle

Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Looks like its working as expected 👍

Please fix the bare excepts in the upstream lib in a followup.

Thanks!

@bdraco bdraco merged commit fe17f97 into home-assistant:dev Jan 20, 2022
@emontnemery
Copy link
Contributor

awesome, thanks @bdraco for testing!

@wjhrdy
Copy link

wjhrdy commented Jan 20, 2022

How do we get access to this bugfix? Is there any configuration or is it transparent to the user?

@kbickar kbickar deleted the sense-59801 branch January 21, 2022 00:00
@kbickar
Copy link
Contributor Author

kbickar commented Jan 21, 2022

@wjhrdy It'll be in the next release or you could take the files changed and put them in your installation

@404FNF
Copy link

404FNF commented Jan 21, 2022

So not in new 2021.12.10 update just installed?

@bdraco
Copy link
Member

bdraco commented Jan 21, 2022

2022.02

@home-assistant home-assistant locked and limited conversation to collaborators Jan 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants