Skip to content

add support for smart pet water dispenser mmgg.pet_waterer.s1 - #1174

Merged
rytilahti merged 12 commits into
rytilahti:masterfrom
ofen:master
Nov 15, 2021
Merged

add support for smart pet water dispenser mmgg.pet_waterer.s1#1174
rytilahti merged 12 commits into
rytilahti:masterfrom
ofen:master

Conversation

@ofen

@ofen ofen commented Nov 2, 2021

Copy link
Copy Markdown
Contributor

#897

Tested on following hardware
Model: mmgg.pet_waterer.s1
Hardware version: esp8266
Firmware version: 2.1.3_0013

@rytilahti rytilahti left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR, @ofen! I added a couple of comments inline. Btw, while developing, you can also call pre-commit run -a (or tox -e lint) to run all the linting tests locally.

Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/miot_device.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
@ofen

ofen commented Nov 3, 2021

Copy link
Copy Markdown
Contributor Author

@ofen

ofen commented Nov 3, 2021

Copy link
Copy Markdown
Contributor Author

Is there any guide how we can fix not-supported device warning in case of MiotDevice class?

Warning due to empty _supported_models list https://github.com/rytilahti/python-miio/blob/master/miio/device.py#L152-L157

@ofen

ofen commented Nov 3, 2021

Copy link
Copy Markdown
Contributor Author

Changes applied according to previous comments. Toggleable boolean method also added for power on/off flow https://github.com/rytilahti/python-miio/pull/1174/files#diff-bda18cd53fbc82e46003cf9e1272b2bff4f747ca8db0dad73ca6f6f2956c98f9R153-R163 but should be discussed (should we use two separate methods or single with boolean switch for this particular case).

@rytilahti

Copy link
Copy Markdown
Owner

Should we use boolean switch for power on/off property too

Turning on and off should be implemented by on() and off() by convention.

Is there any guide how we can fix not-supported device warning in case of MiotDevice class?

This is fixed by defining a class variable like here: https://github.com/rytilahti/python-miio/blob/master/miio/integrations/vacuum/roborock/vacuum.py#L146

…mments + some status properties renamed to show return values e.g. days/minutes
@ofen

ofen commented Nov 3, 2021

Copy link
Copy Markdown
Contributor Author

@rytilahti done

  1. Reverted on() and off() methods back
  2. _supported_models added to fix warning
  3. More comments added
  4. Status properties renamed according to return values
  5. Additional method to reset all filters also added
  6. Property filter_life_time renamed to sponge_filter_life_time for proper semantic with cotton_filter_life_time

@codecov-commenter

codecov-commenter commented Nov 3, 2021

Copy link
Copy Markdown

Codecov Report

Merging #1174 (a9987b4) into master (a19104b) will increase coverage by 0.09%.
The diff coverage is 84.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1174      +/-   ##
==========================================
+ Coverage   78.30%   78.39%   +0.09%     
==========================================
  Files          84       88       +4     
  Lines        9518     9657     +139     
  Branches      775      782       +7     
==========================================
+ Hits         7453     7571     +118     
- Misses       1879     1897      +18     
- Partials      186      189       +3     
Impacted Files Coverage Δ
miio/integrations/petwaterdispenser/device.py 68.51% <68.51%> (ø)
miio/integrations/petwaterdispenser/status.py 96.07% <96.07%> (ø)
miio/__init__.py 100.00% <100.00%> (ø)
miio/integrations/petwaterdispenser/__init__.py 100.00% <100.00%> (ø)
...ntegrations/petwaterdispenser/tests/test_status.py 100.00% <100.00%> (ø)
...o/integrations/vacuum/roborock/vacuumcontainers.py 76.70% <0.00%> (-0.63%) ⬇️
miio/integrations/yeelight/__init__.py 87.78% <0.00%> (-0.17%) ⬇️
miio/integrations/yeelight/spec_helper.py 100.00% <0.00%> (ø)
...ations/yeelight/tests/test_yeelight_spec_helper.py 100.00% <0.00%> (ø)
miio/airhumidifier_jsq.py 95.23% <0.00%> (+0.14%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a19104b...a9987b4. Read the comment docs.

@rytilahti rytilahti left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looking good, here another round of comments! Please do not forget to add an entry to README.md's supported devices list.

Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
Comment thread miio/pet_water_dispenser.py Outdated
…rns `timedelta` objects + code clean-up + status() return object comment + README update
@ofen

ofen commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

@rytilahti all above done

@rytilahti rytilahti left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looking great!

One last wish, could you at least add some tests to the PetWaterDispenserStatus? Nothing fancy is needed here, simply verifying that that the example payload is parsed correctly:

data = (the example response)

def test_status():
    status = PetWaterDispenserStatus(data)

    assert status.is_on is  True
    assert status.sponge_filter_left_days == timedelta(days=10)
    # and so on

Also, instead of using __init__.py as the main code file, it'd be better to store the code elsewhere and just import the exported symbols (e.g., PetWaterDispenser) in side the __init__.py. I notice that this is not properly done in other integrations either, so sorry for that confusion.

@ofen

ofen commented Nov 14, 2021

Copy link
Copy Markdown
Contributor Author

@rytilahti moved code from __init__.py to device.py and status.py files, status test also added.

@rytilahti rytilahti left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM, thanks! 🎉

@rytilahti
rytilahti merged commit a64c66a into rytilahti:master Nov 15, 2021
@rytilahti rytilahti mentioned this pull request Nov 30, 2021
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