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

Update Hue data fetching #31338

Merged
merged 5 commits into from
Jan 31, 2020
Merged

Update Hue data fetching #31338

merged 5 commits into from
Jan 31, 2020

Conversation

balloob
Copy link
Member

@balloob balloob commented Jan 31, 2020

Breaking change

Proposed change

Hue had the problem that if the initial connection didn't fetch lights (because of an error, etc), it would never keep polling for updates! So if first start broke, all was broken. Identified by @walnerz here #23796 (comment)

So I redid the data fetching, inspired by the work I did for Ring. I did this by introducing a generic data coordinator class that is able to take subscribers, scan at an interval and request syncs on demand, distributing the data accordingly.

The tests hardly changed, which makes reviewing easy 👍

Fixes #30864
Fixes #30669
Fixes #29179
Fixes #29337
Fixes #23796

@bramkragten I have requested your review. The tests barely needed any changes but I am unable to test the Hue sensor. I have verified lights work 👍

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

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@balloob balloob added this to the 0.105.0 milestone Jan 31, 2020
@balloob balloob requested a review from a team as a code owner January 31, 2020 06:40
@project-bot project-bot bot added this to Needs review in Dev Jan 31, 2020
@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev Jan 31, 2020
homeassistant/components/hue/light.py Show resolved Hide resolved
homeassistant/components/hue/sensor_base.py Outdated Show resolved Hide resolved
homeassistant/components/hue/sensor_base.py Outdated Show resolved Hide resolved
homeassistant/components/hue/sensor_base.py Outdated Show resolved Hide resolved
homeassistant/components/hue/sensor_base.py Outdated Show resolved Hide resolved
homeassistant/helpers/update_coordinator.py Outdated Show resolved Hide resolved
Dev automation moved this from By Code Owner to Review in progress Jan 31, 2020
@bramkragten
Copy link
Member

Seems to work fine with both lights and sensors 👍

@codecov
Copy link

codecov bot commented Jan 31, 2020

Codecov Report

Merging #31338 into dev will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #31338      +/-   ##
==========================================
- Coverage   94.62%   94.60%   -0.02%     
==========================================
  Files         745      747       +2     
  Lines       54046    54135      +89     
==========================================
+ Hits        51143    51217      +74     
- Misses       2903     2918      +15     
Impacted Files Coverage Δ
homeassistant/components/hue/sensor.py 90.32% <0.00%> (-3.02%) ⬇️
homeassistant/components/vizio/media_player.py 100.00% <0.00%> (ø) ⬆️
homeassistant/components/emulated_hue/hue_api.py 90.51% <0.00%> (ø) ⬆️
homeassistant/components/device_tracker/legacy.py 99.14% <0.00%> (ø) ⬆️
homeassistant/helpers/debounce.py 71.79% <0.00%> (ø)
homeassistant/helpers/update_coordinator.py 91.89% <0.00%> (ø)
homeassistant/helpers/service.py 92.47% <0.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 611127a...897c835. Read the comment docs.

@balloob balloob merged commit 166d770 into dev Jan 31, 2020
Dev automation moved this from Review in progress to Done Jan 31, 2020
@delete-merged-branch delete-merged-branch bot deleted the hue-updates branch January 31, 2020 22:47
balloob added a commit that referenced this pull request Jan 31, 2020
* Refactor Hue Lights to use DataCoordinator

* Redo how Hue updates data

* Address comments

* Inherit from Entity and remove pylint disable

* Add tests for debounce
@lock lock bot locked and limited conversation to collaborators Feb 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.