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

Emulate color temperature for non-ct lights in light groups #23495

Merged
merged 10 commits into from Sep 15, 2019

Conversation

@bryanyork
Copy link
Contributor

commented Apr 28, 2019

Description:

When the light group component is used, we aggregate all the supported features and expose that. This has the unfortunate side effect of causing the grouped light component to send a color temperature to a light that does not support this feature. This is a workaround that converts the color temperature value to HS values for non-supported lights in a light group. (Also fixed one typo I created in an older pull request.)

Related issue (if applicable): fixes #

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

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:

  • The manifest file has all fields filled out correctly (example).
  • New dependencies have been added to requirements in the manifest (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.
@ghost

This comment has been minimized.

Copy link

commented Apr 28, 2019

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (group) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

@balloob

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

I think that it does belong here Pascal. The Light integration should be very strict and not try to be "smart" with those things. The group platform could be a little smarter, as it's already doing smarts to group things.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

@bryanyork are you planning to continue here?

@bryanyork

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@MartinHjelmare Yes, let me address the review above and submit later today.

@MartinHjelmare MartinHjelmare added this to Review in progress in Dev Jul 23, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@bryanyork please close this PR and you can reopen it yourself when ready to finish. We're trying to decrease our open PR buffer. Thanks!

@bryanyork bryanyork force-pushed the bryanyork:ct-group-lights branch from 52a2608 to faf404f Aug 11, 2019
@bryanyork bryanyork force-pushed the bryanyork:ct-group-lights branch from faf404f to 5fc1287 Aug 11, 2019
bryanyork added 2 commits Aug 11, 2019
@bryanyork

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@MartinHjelmare @balloob ready for re-review.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

@balloob please continue the review.

@balloob
balloob approved these changes Sep 5, 2019
Copy link
Member

left a comment

Looks good. Can be merged when final few minor comments have been addressed.

Dev automation moved this from Review in progress to Reviewer approved Sep 5, 2019
bryanyork added 3 commits Sep 15, 2019
@bryanyork

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

@MartinHjelmare @balloob Ready to merge.

Dev automation moved this from Reviewer approved to Review in progress Sep 15, 2019
@bryanyork bryanyork requested a review from MartinHjelmare Sep 15, 2019
Copy link
Member

left a comment

Good!

Dev automation moved this from Review in progress to Reviewer approved Sep 15, 2019
@MartinHjelmare MartinHjelmare merged commit f45f8f2 into home-assistant:dev Sep 15, 2019
11 checks passed
11 checks passed
CI Build #20190915.30 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 93.98%)
Details
codecov/project 93.98% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 15, 2019
ochlocracy added a commit to ochlocracy/home-assistant that referenced this pull request Sep 30, 2019
…istant#23495)

* Emulate color temperature for non-ct lights in light groups

* fix tests

* Address review comments

* Fix black formatting

* Fix for pylint

* Address comments

* Fix black formatting

* Address comments
ochlocracy added a commit to ochlocracy/home-assistant that referenced this pull request Oct 1, 2019
…istant#23495)

* Emulate color temperature for non-ct lights in light groups

* fix tests

* Address review comments

* Fix black formatting

* Fix for pylint

* Address comments

* Fix black formatting

* Address comments
@balloob balloob referenced this pull request Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Done
5 participants
You can’t perform that action at this time.