Reset consumable by name #115

mrin · 2017-11-06T21:57:41Z

Available consumable names:

main_brush_work_time
side_brush_work_time
sensor_dirty_time
filter_work_time

CLI example:
miio reset_consumable sensor_dirty_time

Tested with my robot.

… sensor_dirty_time, filter_work_time

houndci-bot · 2017-11-06T21:57:46Z

miio/vacuum.py

        """Reset consumable information."""
-        raise NotImplementedError("unknown parameters")
-        # self.send("reset_consumable", ["unknown"])
+        # name = ["main_brush_work_time", "side_brush_work_time", "sensor_dirty_time" or "filter_work_time"]


line too long (108 > 79 characters)

Could you create a Consumable enum, and use it instead of passing strings around. See https://github.com/rytilahti/python-miio/blob/master/miio/vacuum.py#L20 (and its use in https://github.com/rytilahti/python-miio/blob/master/miio/vacuum.py#L164) for example.

We do not need to follow their naming scheme either, I think it makes sense here to go for the same naming as in ConsumableStatus (https://github.com/rytilahti/python-miio/blob/master/miio/vacuumcontainers.py#L284).

houndci-bot · 2017-11-06T21:57:46Z

miio/vacuum.py

        """Reset consumable information."""
-        raise NotImplementedError("unknown parameters")
-        # self.send("reset_consumable", ["unknown"])
+        # name = ["main_brush_work_time", "side_brush_work_time", "sensor_dirty_time" or "filter_work_time"]


line too long (108 > 79 characters)

coveralls · 2017-11-06T21:59:31Z

Coverage decreased (-0.1%) to 45.071% when pulling aa3338c on mrin:reset_consumable into 23a6aae on rytilahti:master.

rytilahti

Thanks for the PR! This will be good to have around when it's time for a filter change. I added some slight commentary which should be done before getting this merged.

rytilahti · 2017-11-07T19:44:07Z

miio/vacuum_cli.py

+@pass_dev
+def reset_consumable(vac: miio.Vacuum, name):
+    """Query and reset consumable."""
+    click.echo("Reset consumable %s" % name)


"Reseting consumable '%s'" would look here nicer.

rytilahti · 2017-11-07T19:44:25Z

miio/vacuum.py

-        raise NotImplementedError("unknown parameters")
-        # self.send("reset_consumable", ["unknown"])
+        # name = ["main_brush_work_time", "side_brush_work_time", "sensor_dirty_time" or "filter_work_time"]
+        return self.send("reset_consumable", [name])


Do you happen to know if multiple consumables can be reseted at once?

I not tried this case on my robo.

rytilahti · 2017-11-07T19:46:01Z

miio/vacuum.py

        """Reset consumable information."""
-        raise NotImplementedError("unknown parameters")
-        # self.send("reset_consumable", ["unknown"])
+        # name = ["main_brush_work_time", "side_brush_work_time", "sensor_dirty_time" or "filter_work_time"]


Could you create a Consumable enum, and use it instead of passing strings around. See https://github.com/rytilahti/python-miio/blob/master/miio/vacuum.py#L20 (and its use in https://github.com/rytilahti/python-miio/blob/master/miio/vacuum.py#L164) for example.

We do not need to follow their naming scheme either, I think it makes sense here to go for the same naming as in ConsumableStatus (https://github.com/rytilahti/python-miio/blob/master/miio/vacuumcontainers.py#L284).

houndci-bot · 2017-11-07T21:10:40Z

miio/vacuum_cli.py

@@ -445,6 +465,6 @@ def raw_command(vac: miio.Vacuum, cmd, parameters):
    click.echo("Sending cmd %s with params %s" % (cmd, params))
    click.echo(vac.raw_command(cmd, params))

-
+cli()


expected 2 blank lines after class or function definition, found 1

houndci-bot · 2017-11-07T21:10:40Z

miio/vacuum_cli.py

+    else:
+        return
+
+    click.echo("Resetting consumable '%s': %s" % (name, vac.consumable_reset(consumable)))


line too long (90 > 79 characters)

houndci-bot · 2017-11-07T21:10:40Z

miio/vacuum.py

@@ -21,6 +21,12 @@ class TimerState(enum.Enum):
    On = "on"
    Off = "off"

+class Consumable(enum.Enum):


expected 2 blank lines, found 1

coveralls · 2017-11-07T21:12:39Z

Coverage decreased (-0.2%) to 45.0% when pulling 1ce83c2 on mrin:reset_consumable into 23a6aae on rytilahti:master.

coveralls · 2017-11-07T21:17:33Z

Coverage decreased (-0.2%) to 45.018% when pulling de89ac1 on mrin:reset_consumable into 23a6aae on rytilahti:master.

houndci-bot · 2017-11-07T21:32:01Z

miio/vacuum_cli.py

+        return
+
+    click.echo("Resetting consumable '%s': %s" % (name,
+                                                  vac.consumable_reset(consumable)))


line too long (84 > 79 characters)

coveralls · 2017-11-07T21:40:43Z

Coverage decreased (-0.2%) to 45.018% when pulling d6557ee on mrin:reset_consumable into 23a6aae on rytilahti:master.

coveralls · 2017-11-07T21:42:20Z

Coverage decreased (-0.2%) to 45.018% when pulling 3d1b833 on mrin:reset_consumable into 23a6aae on rytilahti:master.

rytilahti · 2017-11-07T21:41:01Z

miio/vacuum_cli.py

+    elif name == 'sensor_dirty':
+        consumable = Consumable.SensorDirty
+    else:
+        return


I think it makes sense to error out here before returning, if not just to avoid bug reports caused by potential misspellings / name changes.

rytilahti · 2017-11-07T21:43:55Z

miio/vacuum_cli.py

+@click.argument('name', type=str, required=True)
+@pass_dev
+def reset_consumable(vac: miio.Vacuum, name):
+    """Reset consumable."""


Could you also add a list information about the available values? Adding an empty line after the "title line" will make it only visible when help on the specific command is requested, IIRC, so I suppose something like this would work:

"""Reset consumable state. Allowed values: main_brush - explanation another one - x """

houndci-bot · 2017-11-08T19:03:19Z

miio/vacuum_cli.py

+    elif name == 'sensor_dirty':
+        consumable = Consumable.SensorDirty
+    else:
+        click.echo("Allowed values: main_brush, side_brush, filter, sensor_dirty")


line too long (82 > 79 characters)

coveralls · 2017-11-08T19:06:33Z

Coverage decreased (-0.2%) to 45.0% when pulling 120de89 on mrin:reset_consumable into 23a6aae on rytilahti:master.

rytilahti · 2017-11-09T00:21:20Z

Thanks a lot! I adjusted the help text a bit.

coveralls · 2017-11-09T00:22:24Z

Coverage decreased (-0.2%) to 45.0% when pulling 2bbb532 on mrin:reset_consumable into 23a6aae on rytilahti:master.

Reset consumable by name: main_brush_work_time, side_brush_work_time,…

aa3338c

… sensor_dirty_time, filter_work_time

houndci-bot reviewed Nov 6, 2017

View reviewed changes

rytilahti requested changes Nov 7, 2017

View reviewed changes

Refactoring reset consumable

1ce83c2

houndci-bot reviewed Nov 7, 2017

View reviewed changes

Fix cli

de89ac1

Fix coding style for houndci

d6557ee

houndci-bot reviewed Nov 7, 2017

View reviewed changes

Fix coding style for houndci

3d1b833

rytilahti reviewed Nov 7, 2017

View reviewed changes

Update help info for reset_consumable

120de89

houndci-bot reviewed Nov 8, 2017

View reviewed changes

slight revise for reset_consumables help

2bbb532

rytilahti approved these changes Nov 9, 2017

View reviewed changes

rytilahti merged commit ed01fd8 into rytilahti:master Nov 9, 2017

mrin deleted the reset_consumable branch November 9, 2017 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset consumable by name #115

Reset consumable by name #115

mrin commented Nov 6, 2017

houndci-bot Nov 6, 2017

rytilahti Nov 7, 2017

houndci-bot Nov 6, 2017

coveralls commented Nov 6, 2017 •

edited

rytilahti left a comment •

edited

rytilahti Nov 7, 2017

rytilahti Nov 7, 2017

mrin Nov 7, 2017

rytilahti Nov 7, 2017

houndci-bot Nov 7, 2017

houndci-bot Nov 7, 2017

houndci-bot Nov 7, 2017

coveralls commented Nov 7, 2017 •

edited

coveralls commented Nov 7, 2017 •

edited

houndci-bot Nov 7, 2017

coveralls commented Nov 7, 2017 •

edited

coveralls commented Nov 7, 2017 •

edited

rytilahti Nov 7, 2017

rytilahti Nov 7, 2017 •

edited

houndci-bot Nov 8, 2017

coveralls commented Nov 8, 2017 •

edited

rytilahti commented Nov 9, 2017

coveralls commented Nov 9, 2017 •

edited

Reset consumable by name #115

Reset consumable by name #115

Conversation

mrin commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 6, 2017 • edited

rytilahti left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 7, 2017 • edited

coveralls commented Nov 7, 2017 • edited

Choose a reason for hiding this comment

coveralls commented Nov 7, 2017 • edited

coveralls commented Nov 7, 2017 • edited

Choose a reason for hiding this comment

rytilahti Nov 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 8, 2017 • edited

rytilahti commented Nov 9, 2017

coveralls commented Nov 9, 2017 • edited

coveralls commented Nov 6, 2017 •

edited

rytilahti left a comment •

edited

coveralls commented Nov 7, 2017 •

edited

coveralls commented Nov 7, 2017 •

edited

coveralls commented Nov 7, 2017 •

edited

coveralls commented Nov 7, 2017 •

edited

rytilahti Nov 7, 2017 •

edited

coveralls commented Nov 8, 2017 •

edited

coveralls commented Nov 9, 2017 •

edited