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

Add support for Yeelight Dual Control Module (yeelink.switch.sw1) #887

Merged
merged 1 commit into from
Jan 8, 2021
Merged

Add support for Yeelight Dual Control Module (yeelink.switch.sw1) #887

merged 1 commit into from
Jan 8, 2021

Conversation

IhorSyerkov
Copy link
Contributor

@IhorSyerkov IhorSyerkov commented Dec 6, 2020 •
edited
Loading

Added support Yeelight Dual Control Module (yeelink.switch.sw1) from #627 list.
This switch doesn’t have LAN-mode and cannot be controlled via python-yeelight

@IhorSyerkov IhorSyerkov changed the title Added Yeelight Dual Control Module (yeelink.switch.sw1) (from #627 list) WIP: Added Yeelight Dual Control Module (yeelink.switch.sw1) (from #627 list) Dec 6, 2020
@IhorSyerkov IhorSyerkov force-pushed the feature/yeelight_dual_module branch 3 times, most recently from c5379f6 to 776b23c Compare December 7, 2020 18:22
@IhorSyerkov IhorSyerkov marked this pull request as draft December 7, 2020 18:31
@IhorSyerkov IhorSyerkov changed the title WIP: Added Yeelight Dual Control Module (yeelink.switch.sw1) (from #627 list) Added Yeelight Dual Control Module (yeelink.switch.sw1) (from #627 list) Dec 7, 2020
@IhorSyerkov IhorSyerkov marked this pull request as ready for review December 7, 2020 18:31
@IhorSyerkov IhorSyerkov force-pushed the feature/yeelight_dual_module branch from 776b23c to 1dd50f1 Compare December 7, 2020 18:34
rytilahti
rytilahti requested changes Dec 27, 2020
View reviewed changes
Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I added some preliminary comments inline. Could you also add some tests?

miio/yeelight_dual_switch.py Outdated Show resolved Hide resolved
miio/yeelight_dual_switch.py Outdated Show resolved Hide resolved
miio/yeelight_dual_switch.py Show resolved Hide resolved
miio/yeelight_dual_switch.py Outdated Show resolved Hide resolved
miio/yeelight_dual_switch.py Outdated Show resolved Hide resolved
miio/yeelight_dual_switch.py Outdated Show resolved Hide resolved
@IhorSyerkov IhorSyerkov force-pushed the feature/yeelight_dual_module branch 2 times, most recently from 80b12ef to 63bae67 Compare December 30, 2020 18:52
@IhorSyerkov
Copy link
Contributor Author

@rytilahti I updated PR, including all your comments. It's ready for review

rytilahti
rytilahti requested changes Jan 5, 2021
View reviewed changes
Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Sorry for the delay!

A couple of minor things:

  • Docstrings & format_output()s should be consistent with the casing used by other integrations.
  • Check out if the get_properties_for_mapping() works for this device

miio/yeelight_dual_switch.py Outdated Show resolved Hide resolved
miio/tests/dummies.py Show resolved Hide resolved
miio/yeelight_dual_switch.py Show resolved Hide resolved
miio/yeelight_dual_switch.py Outdated Show resolved Hide resolved
miio/yeelight_dual_switch.py Outdated Show resolved Hide resolved
@IhorSyerkov IhorSyerkov force-pushed the feature/yeelight_dual_module branch from 63bae67 to 8aabf4f Compare January 8, 2021 21:08
@IhorSyerkov IhorSyerkov requested a review from rytilahti January 8, 2021 21:12
rytilahti
rytilahti approved these changes Jan 8, 2021
View reviewed changes
Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

LGTM! If the API feels "odd", we can always improve on that in the future, thanks for the PR! 🥇

rytilahti
rytilahti requested changes Jan 8, 2021
View reviewed changes
miio/yeelight_dual_switch.py Outdated Show resolved Hide resolved
miio/yeelight_dual_switch.py Show resolved Hide resolved
@IhorSyerkov IhorSyerkov force-pushed the feature/yeelight_dual_module branch 2 times, most recently from dff66c8 to ae5c854 Compare January 8, 2021 21:25
@IhorSyerkov IhorSyerkov force-pushed the feature/yeelight_dual_module branch from ae5c854 to db9e4bb Compare January 8, 2021 21:27
@IhorSyerkov IhorSyerkov requested a review from rytilahti January 8, 2021 21:28
rytilahti
rytilahti approved these changes Jan 8, 2021
View reviewed changes
Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

👍 can be merged as soon as the tests pass, thanks again!

@rytilahti rytilahti changed the title Added Yeelight Dual Control Module (yeelink.switch.sw1) (from #627 list) Add support for Yeelight Dual Control Module (yeelink.switch.sw1) Jan 8, 2021
@rytilahti rytilahti merged commit 29feacb into rytilahti:master Jan 8, 2021
@nick2525
Copy link

Can your please link home assistant pull request, or the pr has not yet been created in the home assistant ?

@IhorSyerkov
Copy link
Contributor Author

IhorSyerkov commented Jan 26, 2021 •
edited
Loading

I didn't create HA PR yet. But planning to do it.
Waiting for next release.

@rytilahti rytilahti mentioned this pull request Mar 13, 2021
Merged
xvlady pushed a commit to xvlady/python-miio that referenced this pull request May 9, 2021
@IhorSyerkov @xvlady
Add support for Yeelight Dual Control Module (yeelink.switch.sw1) (ry…
c283e08 
…tilahti#887)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rytilahti rytilahti rytilahti approved these changes

Assignees
No one assigned
Labels
miot new device
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@IhorSyerkov @nick2525 @rytilahti