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.

Sign up for GitHub

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

Jump to bottom

Adding support for GU10 lights #141

Merged
merged 18 commits into from
Jan 21, 2018
Merged

Adding support for GU10 lights #141

merged 18 commits into from
Jan 21, 2018

Conversation

ggravlingen
Copy link
Member

@ggravlingen ggravlingen commented Jan 19, 2018 •
edited
Loading

Closes #135

houndci-bot
houndci-bot reviewed Jan 19, 2018
View reviewed changes
tests/devices.py Outdated
"5709":30138,
"5710":26909,
"5706":"efd275",
"9003":0
Copy link

Choose a reason for hiding this comment

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

missing whitespace after ':'

tests/devices.py Outdated
"5711":450,
"5709":30138,
"5710":26909,
"5706":"efd275",
Copy link

Choose a reason for hiding this comment

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

missing whitespace after ':'

tests/devices.py Outdated
"5717":0,
"5711":450,
"5709":30138,
"5710":26909,
Copy link

Choose a reason for hiding this comment

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

missing whitespace after ':'

tests/devices.py Outdated
"5850":0,
"5717":0,
"5711":450,
"5709":30138,
Copy link

Choose a reason for hiding this comment

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

missing whitespace after ':'

tests/devices.py Outdated
"5851":10,
"5850":0,
"5717":0,
"5711":450,
Copy link

Choose a reason for hiding this comment

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

missing whitespace after ':'

tests/devices.py Outdated
"3":{
"0":"IKEA of Sweden",
"1":"TRADFRI bulb GU10 WS 400lm",
"2":"",
Copy link

Choose a reason for hiding this comment

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

missing whitespace after ':'

tests/devices.py Outdated
"9019":1,
"3":{
"0":"IKEA of Sweden",
"1":"TRADFRI bulb GU10 WS 400lm",
Copy link

Choose a reason for hiding this comment

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

missing whitespace after ':'

tests/devices.py Outdated
"9054":0,
"9019":1,
"3":{
"0":"IKEA of Sweden",
Copy link

Choose a reason for hiding this comment

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

missing whitespace after ':'

tests/devices.py Outdated
"9003":65540,
"9054":0,
"9019":1,
"3":{
Copy link

Choose a reason for hiding this comment

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

missing whitespace after ':'

tests/devices.py Outdated
"9020":1516008285,
"9003":65540,
"9054":0,
"9019":1,
Copy link

Choose a reason for hiding this comment

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

missing whitespace after ':'

@ggravlingen ggravlingen changed the title Adding support for GU10 lights WIP: Adding support for GU10 lights Jan 19, 2018
@ggravlingen ggravlingen mentioned this pull request Jan 19, 2018
Closed
lwis
lwis reviewed Jan 19, 2018
View reviewed changes
pytradfri/device.py Outdated
class LightControl:
"""Class to control the lights."""
class DeviceControl:
"""Superclass to control lights."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

to control devices.

Copy link
Member Author

@ggravlingen ggravlingen Jan 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I've changed this now

@coveralls
Copy link

coveralls commented Jan 20, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 60.74% when pulling c0c0711 on fixswitch into 807ef60 on master.

@coveralls
Copy link

coveralls commented Jan 20, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 60.74% when pulling aeac3a5 on fixswitch into 807ef60 on master.

lwis
lwis reviewed Jan 20, 2018
View reviewed changes
pytradfri/device.py Outdated
"""Return raw data that it represents.
If regular bulb=>ATTR_LIGHT_CONTROL If GU10 light=>ROOT_SWITCH."""
if ATTR_LIGHT_CONTROL in self._device.raw:
return self._device.raw[ATTR_LIGHT_CONTROL]
Copy link
Collaborator

@lwis lwis Jan 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The type should come from another func, maybe _device_type? This then means we only have one place we do the selection.

I feel that in the future device.light_control and device.has_light_control will be confusing, so we may need to move this logic up to Device, which can then return the appropriate Control class for the type, making DeviceControl the super.

Then we'd have three functions on Device;

  • device_control (I don't care what type it is, I just want to be able to switch the device on/off)
  • light_control (I want light specific control functions)
  • switch_control (I want switch specific functions, whatever these may be)

However, it's very frustrating that the GU10 bulb is identified as a switch, as this makes the API very confusing to a user, I'm not sure there is a neat way for us to handle this.

The way this stands today, if IKEA release a switch tomorrow the API will allow someone to change the brightness on a switch... Not ideal.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lwis thanks! I will give this some more consideration and revert. On switches, I believe they call their "switch" a "plug", so it might be put in a whole different root path.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lwis should we wait for the plugs to be released before we do the reshuffling?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's wise, as we'll have to do some work to support it anyway.

@coveralls
Copy link

coveralls commented Jan 20, 2018 •
edited
Loading

Coverage Status

Coverage increased (+3.2%) to 63.72% when pulling 81bd8c9 on fixswitch into 807ef60 on master.

@coveralls
Copy link

coveralls commented Jan 20, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 63.72% when pulling 3e89568 on fixswitch into 0a3b4e2 on master.

@ggravlingen
Copy link
Member Author

@lwis ok, let's talk more on structure of classes later on. Good to merge once it's been user tested?

@lwis
Copy link
Collaborator

lwis commented Jan 21, 2018

LGTM

@ggravlingen
Copy link
Member Author

👍🏻 (I had to look that one up in urban dictionary 🙈)

@ggravlingen ggravlingen changed the title WIP: Adding support for GU10 lights Adding support for GU10 lights Jan 21, 2018
@ggravlingen ggravlingen merged commit b252867 into master Jan 21, 2018
@ggravlingen ggravlingen deleted the fixswitch branch January 21, 2018 15:43
@Lakitna Lakitna mentioned this pull request Jan 31, 2018
Closed
@coveralls coveralls mentioned this pull request Jan 31, 2018
Merged
ggravlingen pushed a commit that referenced this pull request Feb 1, 2018
@Lakitna
Partial reversal of PR #141 (#151)
f2a11b2 
* Fixed has_light_control check
* Removed GU10 specific things from tests. The GU10 profile was that of a wrongly paired lamp. We shouldn't test that.
* Improved coverage for device.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lwis lwis lwis left review comments

@houndci-bot houndci-bot houndci-bot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ggravlingen @coveralls @lwis @houndci-bot