Merge of the Chuangmi Plug V1, V2, V3 and M1 by syssi · Pull Request #270 · rytilahti/python-miio

syssi · 2018-03-16T21:38:28Z

@rytilahti Could you provide a recommendation for the "device model enum" and the discovery device map?

coveralls · 2018-03-16T21:40:38Z

Coverage decreased (-0.2%) to 69.043% when pulling b2e3965 on syssi:feature/marriage-chuangmi-v1-and-v3 into 17e8e69 on rytilahti:master.

coveralls · 2018-03-16T21:40:38Z

Coverage decreased (-0.4%) to 68.847% when pulling 7685d79 on syssi:feature/marriage-chuangmi-v1-and-v3 into 020b861 on rytilahti:master.

rytilahti

I like the diffs which remove more than they add, well done! I added a couple of comments inline.

rytilahti · 2018-03-16T21:44:02Z

miio/device.py

    it should be used instead of it."""
    def __init__(self, ip: str = None, token: str = None,
-                 start_id: int=0, debug: int=0, lazy_discover: bool=True) -> None:
+                 start_id: int=0, debug: int=0, lazy_discover: bool=True, model: str=None) -> None:


Is it truly necessary to introduce this also on the parent level, or would it be fine to have it just in the subclass?

rytilahti · 2018-03-16T21:45:07Z

miio/discovery.py

-    "chuangmi-plug-v3": PlugV3,
+    "chuangmi-plug-v1": ChuangmiPlug,
+    "chuangmi-plug_": ChuangmiPlug,
+    "chuangmi-plug-v3": ChuangmiPlug,


For passing a parameter here you can use functools.partial, so something like:

partial(ChuangmiPlug, model=MyModel)

can be used in this case.

I'm wondering whether that should be an enum, or simply an integer model_version, which would default to either of them? We could also derive the model (if not given) per default from the status information (to adapt the setters accordingly), as it's just one extra attribute & different naming?

I will care about tomorrow.

The check_and_create_device method doesn't enter the inspect.isclass(v) branch anymore if I use a partial. I'm unsure about a good implementation.

Indeed.. Hmm, maybe adding a check if it's partial and extracting the class out from it?

>>> from functools import partial >>> class A: >>> pass >>> partial(A, foo=1).func <class '__main__.A'>

edit: that does interfere with the iscallable check.. It shouldn't be a problem though, when the check order is:

Is a class

Is a partial, containing a class

Is callable

rytilahti · 2018-03-16T21:45:22Z

miio/__init__.py

 from miio.protocol import Message, Utils
 from miio.vacuumcontainers import (VacuumStatus, ConsumableStatus, DNDStatus,
-                                   CleaningDetails, CleaningSummary, Timer)
+                                   CleaningDetails, CleaningSummary, Timer, )


Is this a necessary change?

houndci-bot · 2018-03-16T22:01:38Z

miio/chuangmi_plug.py

+    MODEL_CHUANGMI_PLUG_M1: ['power', 'temperature']
+}
+
+class ChuangmiPlugStatus:


expected 2 blank lines, found 1

houndci-bot · 2018-03-16T22:06:45Z

miio/device.py

            self.token = bytes.fromhex(token)
        self.debug = debug
        self.lazy_discover = lazy_discover
+        self.model = model


undefined name 'model'

rytilahti · 2018-03-17T13:44:03Z

One open question is whether we want to keep backwards compatibility with the old class naming? If yes, that could be simply done by creating small wrapper classes located inside chuangmi_plug, which are also exported (and yielding a warning about deprecation).

syssi · 2018-03-24T10:37:39Z

Could you review the changes? I don't know how to improve the tests. The constructor of the old classes should be called and the base class must be mocked somehow.

rytilahti

A couple of minor improvement ideas. After those are changed let's get this merged, the tests can be improved at some later point.

rytilahti · 2018-03-24T22:08:07Z

miio/__init__.py

+from miio.chuangmi_plug import Plug
+from miio.chuangmi_plug import PlugV1
+from miio.chuangmi_plug import PlugV3
+from miio.chuangmi_plug import ChuangmiPlug


from miio.chuangmi_plug import (Plug, PlugV1, PlugV3, ChuangiPlug) would be nicer here.

rytilahti · 2018-03-24T22:24:55Z

miio/chuangmi_plug.py

+                         model=model)
+
+
+class Plug(ClassDeprecatedPlug):


I created PR #276 to move the deprecated decorator out from vacuumcontainers, it should be used to decorate these classes to inform anyone trying to initialize these classes.

Comma removed.

This reverts commit 7eb1443.

syssi · 2018-03-26T11:55:32Z

Done :-)

houndci-bot · 2018-03-26T11:56:37Z

miio/chuangmi_plug.py

@@ -0,0 +1,181 @@
+import logging
+import warnings


'warnings' imported but unused

remvoe unused warnings.

rytilahti · 2018-03-26T19:51:39Z

Great, thanks! 💯

rytilahti reviewed Mar 16, 2018

View reviewed changes

houndci-bot reviewed Mar 16, 2018

View reviewed changes

syssi changed the title ~~Merge of the Chuangmi Plug V1 and V3~~ Merge of the Chuangmi Plug V1, V2, V3 and M1 Mar 16, 2018

rytilahti mentioned this pull request Mar 18, 2018

Xiaomi MiIO Switch: Support for the Xiaomi Chuangmi Plug V3 home-assistant/core#13271

Merged

rytilahti approved these changes Mar 24, 2018

View reviewed changes

syssi added 17 commits March 26, 2018 13:47

Chuangmi Plug V1 and V3 merged.

0b05c24

Chuangmi V1 and V3 merge completed.

8b51166

Whitespace removed

49a6d0e

DummyDevice used in both cases

82f5a1c

Chuangmi Plug M1 merged.

b34af20

Missing blank line added.

3197528

Comma removed.

Model parameter moved to the sub class.

908d77f

Missed line removed.

44155a0

Default model added.

2487e73

Deprecation warning added

a20f468

Test the device class via the deprecation layer

9b86946

Revert "Test the device class via the deprecation layer"

c057b6b

This reverts commit 7eb1443.

Provide a model per mDNS prefix

bd44616

Refactoring

da1a970

Missing parameter added

1b73dd9

Default model added

968bee6

More compact imports

200a738

syssi force-pushed the feature/marriage-chuangmi-v1-and-v3 branch from 3ccef7e to 200a738 Compare March 26, 2018 11:47

Deprecated decorator used

7685d79

houndci-bot reviewed Mar 26, 2018

View reviewed changes

miio/chuangmi_plug.py Outdated

@@ -0,0 +1,181 @@

import logging

import warnings

Copy link
Copy Markdown

houndci-bot Mar 26, 2018

Choose a reason for hiding this comment

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

'warnings' imported but unused

Update chuangmi_plug.py

2fb6a1f

remvoe unused warnings.

rytilahti merged commit 42e6035 into rytilahti:master Mar 26, 2018

Uh oh!

Conversation

syssi commented Mar 16, 2018

Uh oh!

coveralls commented Mar 16, 2018

Uh oh!

coveralls commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rytilahti Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rytilahti commented Mar 17, 2018

Uh oh!

syssi commented Mar 24, 2018

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syssi commented Mar 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rytilahti commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Mar 16, 2018 •

edited

Loading

rytilahti Mar 17, 2018 •

edited

Loading