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

Add support for viomi.waterheater.e1 devices #834

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

Zuz666
Copy link
Contributor

@Zuz666 Zuz666 commented Oct 11, 2020

Just added basic support for 'Viomi Electric Water Heater' devices (viomi.waterheater.e1*): power on/off + status (retrieve properties). Fully tested on real device, works fine.

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! A brief initial review: please add an example response, follow the python naming conventions (foo_bar instead of camelcased fooBar). The property names should be descriptive instead of using the naming used by the protocol itself.

miio/waterheater.py Outdated Show resolved Hide resolved
miio/waterheater.py Outdated Show resolved Hide resolved
miio/waterheater.py Outdated Show resolved Hide resolved
miio/waterheater.py Outdated Show resolved Hide resolved
miio/waterheater.py Outdated Show resolved Hide resolved
@Zuz666
Copy link
Contributor Author

Zuz666 commented Oct 17, 2020

@rytilahti Thanks for great code review! I will make the code better as you request.

@makp0
Copy link

makp0 commented Oct 3, 2021

@Zuz666 , any progress on this pr? 🙂

@Zuz666 Zuz666 changed the title Initial support for viomi.waterheater.e1 devices Add support for viomi.waterheater.e1 devices Nov 5, 2021
@Zuz666
Copy link
Contributor Author

Zuz666 commented Nov 5, 2021

@Zuz666 , any progress on this pr? 🙂

@rytilahti, yep, just check the latest commits!
Fully tested on real device, works fine.

@Zuz666 Zuz666 requested a review from rytilahti November 5, 2021 10:19
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 update! I added a couple of more comments. Could you also do the following:

  • Add an entry to README.md
  • Move the code to be under miio/integrations/heater/viomiwaterheater/ - there are other heaters so we may have a common API among all of them at some point :-)

miio/discovery.py Outdated Show resolved Hide resolved
miio/discovery.py Outdated Show resolved Hide resolved
miio/waterheater.py Outdated Show resolved Hide resolved
miio/waterheater.py Outdated Show resolved Hide resolved
miio/waterheater.py Outdated Show resolved Hide resolved
miio/waterheater.py Outdated Show resolved Hide resolved
miio/waterheater.py Outdated Show resolved Hide resolved
@Zuz666
Copy link
Contributor Author

Zuz666 commented Nov 5, 2021

  • Add an entry to README.md
  • Move the code to be under miio/integrations/heater/viomiwaterheater/ - there are other heaters so we may have a common API among all of them at some point :-)

@rytilahti Job done! Please perform a code review.

@Zuz666 Zuz666 requested a review from rytilahti November 5, 2021 21:00
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.

Looking good, I added a couple of very minor changes.

Could you also please add at least tests to the container functionality (see #1174 (review))?

miio/device.py Outdated Show resolved Hide resolved
miio/integrations/waterheater/viomi/viomiwaterheater.py Outdated Show resolved Hide resolved
miio/integrations/waterheater/viomi/viomiwaterheater.py Outdated Show resolved Hide resolved
miio/integrations/waterheater/viomi/viomiwaterheater.py Outdated Show resolved Hide resolved
miio/integrations/waterheater/viomi/viomiwaterheater.py Outdated Show resolved Hide resolved
miio/integrations/waterheater/viomi/viomiwaterheater.py Outdated Show resolved Hide resolved
@Zuz666 Zuz666 requested a review from rytilahti November 6, 2021 16:42
@Zuz666
Copy link
Contributor Author

Zuz666 commented Nov 6, 2021

Looking good, I added a couple of very minor changes.

Could you also please add at least tests to the container functionality (see #1174 (review))?

Some code for testing done but how can I run this test? Newbie to python. )))

@rytilahti
Copy link
Owner

rytilahti commented Nov 6, 2021

Simply execute pytest and it should pick it automatically (as long as the filename and the test functions follow the test_* pattern) :-) btw, did you forget to add the test file to the PR?

You can also run the linting tests locally (tox -e lint or pre-commit run -a) or the whole test suite (simply tox) if you wish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants