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 check to validate gamut #20518

Merged
merged 15 commits into from Jan 29, 2019

Conversation

@starkillerOG
Copy link
Contributor

starkillerOG commented Jan 27, 2019

Description:

Add a check to the color.util to check if a collor gamut obtained from a device is valid or not.
Also implement this check for the Philips Hue component to prevent fatal errors when a gamut of all 0 is supplied. (apperently that can happen, see issue #20447).

I also added tests in the util.test_color to check if the gamut checker works.

Related issue (if applicable): helps debug #20447 (and also a partial fix)

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@starkillerOG starkillerOG requested a review from home-assistant/core as a code owner Jan 27, 2019

@wafflebot wafflebot bot added the in progress label Jan 27, 2019

Show resolved Hide resolved homeassistant/util/color.py Outdated
Show resolved Hide resolved homeassistant/util/color.py Outdated
Show resolved Hide resolved homeassistant/util/color.py Outdated
Show resolved Hide resolved homeassistant/util/color.py

starkillerOG added some commits Jan 27, 2019

Show resolved Hide resolved tests/util/test_color.py Outdated
Show resolved Hide resolved tests/util/test_color.py Outdated
Show resolved Hide resolved tests/util/test_color.py Outdated
Show resolved Hide resolved tests/util/test_color.py Outdated
Show resolved Hide resolved tests/util/test_color.py Outdated
@starkillerOG

This comment has been minimized.

Copy link
Contributor Author

starkillerOG commented Jan 27, 2019

@rohankapoorcom if Travis check succeeds could you merge this?
It will prevent fatal errors related to issue #20447 (that you pointed out to me).

I tested this code lokally and it works for me.
If Travis fails I will fix it tommorow (their is a long que at the moment).

@rohankapoorcom

This comment has been minimized.

Copy link
Member

rohankapoorcom commented Jan 27, 2019

@starkillerOG I'm not super familiar with gamuts but this looks reasonable to me. I'd prefer if someone else signed off on it before merging though. I will tag it for the next hotfix.

@rohankapoorcom rohankapoorcom added this to the 0.86.4 milestone Jan 27, 2019

@rohankapoorcom

This comment has been minimized.

Copy link
Member

rohankapoorcom commented Jan 28, 2019

@starkillerOG Looks like there's a couple of linting problems as well as some broken tests that will need to be fixed before this can be merged.

@starkillerOG

This comment has been minimized.

Copy link
Contributor Author

starkillerOG commented Jan 28, 2019

@rohankapoorcom thank you for your revieuw!
Will start working on it.

starkillerOG added some commits Jan 28, 2019

@starkillerOG

This comment has been minimized.

Copy link
Contributor Author

starkillerOG commented Jan 28, 2019

@rohankapoorcom it now passes all checks and I have made the 'none' string a constant as suggested.
I think this is now ready to be merged.

def check_valid_gamut(Gamut: GamutType) -> bool:
"""Check if the supplied gamut is valid."""
# Check if gamut is None
if not Gamut:

This comment has been minimized.

@balloob

balloob Jan 28, 2019

Member

We shouldn't check this in here, instead we should not call this function if it's None

This comment has been minimized.

@starkillerOG

starkillerOG Jan 28, 2019

Author Contributor

Alright I will adapt to put the None check in the Hue.light.py and remove it in the util.color.

starkillerOG added some commits Jan 28, 2019

@starkillerOG

This comment has been minimized.

Copy link
Contributor Author

starkillerOG commented Jan 28, 2019

@balloob I included you suggestions, anything else that I can inprove?

@starkillerOG

This comment has been minimized.

Copy link
Contributor Author

starkillerOG commented Jan 28, 2019

Please wait with merging this PR, I might want to edit the warning message.
We just discovered this issue might be because of outdated software of the bulbs/bridge.
So I might want to include a "please update the hue bridge and light." to the error message.

@starkillerOG

This comment has been minimized.

Copy link
Contributor Author

starkillerOG commented Jan 28, 2019

@LandOfDodd just confirmed that a software update of the hue bridge/bulb fixes the issue that this PR is chatching. Theirfore I will include a prompt to update the hue bridge/bulbs if this issue occurs.

Show resolved Hide resolved homeassistant/components/hue/light.py Outdated
Show resolved Hide resolved homeassistant/components/hue/light.py
Show resolved Hide resolved homeassistant/components/hue/light.py Outdated
Show resolved Hide resolved homeassistant/components/hue/light.py
Show resolved Hide resolved homeassistant/components/hue/light.py
Show resolved Hide resolved homeassistant/components/hue/light.py Outdated
Show resolved Hide resolved homeassistant/components/hue/light.py Outdated
Show resolved Hide resolved homeassistant/components/hue/light.py Outdated
Show resolved Hide resolved homeassistant/components/hue/light.py Outdated
Show resolved Hide resolved homeassistant/components/hue/light.py Outdated

@balloob balloob merged commit f353d51 into home-assistant:dev Jan 29, 2019

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 93.023%
Details

@wafflebot wafflebot bot removed the in progress label Jan 29, 2019

balloob added a commit that referenced this pull request Jan 29, 2019

Add check to validate gamut (#20518)
* color.util - Add check to validate gamut

* fix indents

* fix typo

* Add check to validate gamut

* Add tests for gamut checker

* fix test

* fix pylint issues

* fix hue light gamut tests

* add check to validate gamut

* move None check

* Move None check

* Include prompt to update bridge/bulb on error

* fix wrong commit

* fix error message

* Update light.py

@balloob balloob referenced this pull request Jan 29, 2019

Merged

0.86.4 #20559

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment