Skip to content

Support multiple Hue bridges with lights of the same id#11259

Merged
pvizeli merged 2 commits into
home-assistant:devfrom
andreacampi:fix-multiple-bridges
Dec 24, 2017
Merged

Support multiple Hue bridges with lights of the same id#11259
pvizeli merged 2 commits into
home-assistant:devfrom
andreacampi:fix-multiple-bridges

Conversation

@andreacampi
Copy link
Copy Markdown
Contributor

@andreacampi andreacampi commented Dec 20, 2017

Description:

The old code pre-refactoring kept a per-bridge list of lights in a closure; my refactoring moved that to hass.data, which is convenient but caused them to conflict with each other.

Related issue (if applicable): fixes #11183

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

Comment thread tests/components/light/test_hue.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuation line under-indented for visual indent

Comment thread tests/components/light/test_hue.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuation line under-indented for visual indent

Comment thread tests/components/light/test_hue.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (95 > 79 characters)

Comment thread tests/components/light/test_hue.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing whitespace

Comment thread tests/components/light/test_hue.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing whitespace

Comment thread tests/components/light/test_hue.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuation line under-indented for visual indent

Comment thread tests/components/light/test_hue.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuation line under-indented for visual indent

Comment thread tests/components/light/test_hue.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (95 > 79 characters)

@thundergreen
Copy link
Copy Markdown

Could this please be merged in dev to apply this in my setup.

@andreacampi
Copy link
Copy Markdown
Contributor Author

andreacampi commented Dec 21, 2017

@thundergreen I would appreciate you testing this since I only have one bridge right now. You can download the diff from my repo and apply it on top of your dev tree.

That said I just pushed a fix for the lint warnings so this should be ready for review.

…me id.

The old code pre-refactoring kept a per-bridge list of lights in a closure; my refactoring moved that to hass.data, which is convenient but caused them to conflict with each other.

Fixes home-assistant#11183
@thundergreen
Copy link
Copy Markdown

I switched back tob0.59.2 as all kind of modifications won't work for me but that's maybe also my fault
Will have to wait for a fix.sorry

@MartinHjelmare MartinHjelmare changed the title Improve support for multiple Hue bridges with lights that have the sa… Support multiple Hue bridges with lights of the same id Dec 23, 2017
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me. I think the hue light platform could benefit of another refactor to make it more DRY and less convoluted, but that's for another PR. A small comment below.

Comment thread tests/components/light/test_hue.py Outdated

with patch('homeassistant.components.light.hue.get_bridge_type',
return_value=self.mock_bridge_type):
with patch('homeassistant.components.light.hue.HueLight.' +
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the +.

Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to merge after address other comments

@andreacampi
Copy link
Copy Markdown
Contributor Author

@pvizeli I don't think there are other changes pending, after you removed the + (thanks!). Feel free to merge at your convenience.

@pvizeli pvizeli merged commit 8c303bf into home-assistant:dev Dec 24, 2017
@MartinHjelmare MartinHjelmare added this to the 0.60.1 milestone Dec 29, 2017
balloob pushed a commit that referenced this pull request Jan 5, 2018
* Improve support for multiple Hue bridges with lights that have the same id.

The old code pre-refactoring kept a per-bridge list of lights in a closure; my refactoring moved that to hass.data, which is convenient but caused them to conflict with each other.

Fixes #11183

* Update test_hue.py
@balloob balloob mentioned this pull request Jan 5, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hue Bridges on 0.60 and Dev

7 participants