-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
Report unavailable entities to google #28501< 8000 /span>
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
Conversation
Hey there @home-assistant/cloud, mind taking a look at this pull request as its been labeled with a integration ( |
|
||
def convert(mireds): | ||
if mireds is None: | ||
return None | ||
return color_util.color_temperature_mired_to_kelvin(mireds) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat of a workaround. A better solution might be to let the color util return None when input is None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, color util shouldn't catch None
s, it should blow up.
Hehe. Lol. Yea same pull. I've hopefully fixed the failing tests thou. The color temperature ends up None when unavailable. So we must support that case. |
#28228 will break tests on this pull. Needs adjustment once that goes in. |
The 3.6 test fail seem unrelated: |
Wouldn't we still be hit by home-assistant/architecture#297 ? |
Yes, that is why the fix for color conversion is needed. Min/max ends up none. I don't have any light with that synced to cloud, so not sure what GA will do with none values. Now that you say it. Given that it's part of sync call, that trait will likely not work untill next sync. So this will solve it for normal on/off /brightness lights, but not for color setting ones. |
So what about we implement the architecture issue first ? |
I don't mind that. But this same pull will be valid then too. So not sure why wait. It solves issue for some users now. Not sure when I'd have to time to look at the arch issue. Personally rather get the disconnect stuff in shape first. |
Didn't need rebase. Thought brightness was part of sync, but obviously not. I've tried re-triggering test on 3.6. Will se if starts. |
The thing is that min/max mireds will never be |
Entities should only removed when removed from HA. Removing a temporarily unavailable entity from google causes it to need to re-configured once it become available again.
228da6d
to
6a976db
Compare
Description:
Entities should only removed when removed from HA. Removing a temporarily unavailable entity from google causes it to need to re-configured once it become available again.
Related issue (if applicable): fixes #28268
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>
Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
python3 -m script.hassfest
.requirements_all.txt
by runningpython3 -m script.gen_requirements_all
..coveragerc
.If the code does not interact with devices: