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
Warn when lights violate color mode rules #110336
Conversation
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
if not self.platform: | ||
return True | ||
# philips_js and zha have known issues, we don't need users to open issues | ||
return self.platform.platform_name not in {"philips_js", "zha"} |
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.
This return condition is not validated in the tests. Consider adding platform_name to one of the parametrized tests?
Are we better off having these two integrations explicitly opt out of the issue, rather than coding here? This way when the issue is fixed in the integration no changes are required to this base class?
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.
This return condition is not validated in the tests. Consider adding platform_name to one of the parametrized tests
Hmm, you're right 👍 I'll update the tests.
Are we better off having these two integrations explicitly opt out of the issue, rather than coding here
We usually use this pattern to temporarily opt-out of validation, but I guess the integrations could opt out too.
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.
Test update looks good.
Since this is the established pattern that is fine with me, as consistency is important. The risk is if this does not get removed after zha fixes the issue, it could mask the issue being in zha (e.g. it wasn't fixed completely). I'm fine with it as is.
Proposed change
Warn when lights violate color mode rules and ask users to raise issues
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: