Implement adding, deleting and updating the timer by rytilahti · Pull Request #78 · rytilahti/python-miio

rytilahti · 2017-09-15T20:28:26Z

New commands are:

mirobo timer add --cron --command --params
mirobo timer delete
mirobo timer update [--on|--off]

Although the vacuum accepts command and params, the added cron entry will be just 'start_clean'

New commands are: * mirobo timer add --cron <cron string> --command <command> --params <params> * mirobo timer delete <id> * mirobo timer update [--on|--off] <id> Although the vacuum accepts command and params, the added cron entry will be just 'start_clean'

houndci-bot · 2017-09-15T20:28:37Z

mirobo/vacuum.py

+
+    def update_timer(self, timer_id: int, mode):
+        if mode != "on" and mode != "off":
+            raise DeviceException("update_timer needs to be either 'on' or 'off")


line too long (81 > 79 characters)

houndci-bot · 2017-09-15T20:28:38Z

mirobo/vacuum.py

+    def add_timer(self, cron, command, parameters):
+        import time
+        ts = int(round(time.time() * 1000))
+        return self.send("set_timer", [[str(ts), [cron, [command, parameters]]]])


line too long (81 > 79 characters)

coveralls · 2017-09-15T20:30:27Z

Coverage decreased (-0.4%) to 32.645% when pulling f3d5e43 on vacuum_timer into 69ae780 on master.

coveralls · 2017-09-15T20:41:02Z

Coverage decreased (-0.4%) to 32.645% when pulling 52c0c58 on vacuum_timer into 69ae780 on master.

rytilahti · 2017-09-15T21:13:20Z

@syssi please check and feel free to merge as you see fit. Same applies for the readme update in #79!

syssi · 2017-09-16T09:53:53Z

mirobo/vacuum_cli.py

+        #  Note ts == ID for changes
+        click.echo(click.style("Timer #%s, id %s (ts: %s)" % (
+            idx, timer.id, timer.ts), bold=True, fg=color))
+        print("  %s" % timer.cron)


Why do you use print here instead of click.echo?

syssi · 2017-09-16T09:57:37Z

mirobo/vacuum_cli.py

+            idx, timer.id, timer.ts), bold=True, fg=color))
+        print("  %s" % timer.cron)
+        min, hr, x, y, days = timer.cron.split(' ')
+        # hr is in gmt+8 (chinese time), TODO convert to local


Be careful here. I assume the issue will be fixed in future. Users of the mi home app are affected, too.

Since the vacuum supports timezones nowadays, those should be times in active timezone. Removed the conversion and added a print-out for the timezone.

syssi · 2017-09-16T10:02:06Z

mirobo/vacuum_cli.py

+@click.option('--off', is_flag=True)
+@pass_dev
+def update(vac: mirobo.Vacuum, timer_id, on, off):
+    """Update (on or off) an existing scheduled."""


Enable/Disable a stored timer. I would prefer "--enable" and "--disable" as well.

Changed to --enable and --disable, """Enable/disable a timer.""" as help message to be consistent with other commands.

syssi · 2017-09-16T10:03:52Z

mirobo/vacuum.py

+        return self.send("del_timer", [str(timer_id)])
+
+    def update_timer(self, timer_id: int, mode):
+        if mode != "on" and mode != "off":


You would suggest a "enum class" here. ;-)

I would indeed, was thinking about whether to have it for a boolean here. I changed it by adding Vacuum.TimerState {On, Off} ;-)

coveralls · 2017-09-16T14:47:41Z

Coverage decreased (-0.3%) to 32.784% when pulling 6398796 on vacuum_timer into 69ae780 on master.

syssi · 2017-09-16T15:29:40Z

README.md

+Activating/deactivating an existing timer,
+use `mirobo timer` to get the required id.
+```
+$ mirobo timer update <id> [--off|--on]


You missed to update this line.

Both are fixed now, thanks!

syssi · 2017-09-16T15:30:50Z

mirobo/vacuum_cli.py

-            hr = (int(hr) - 8) % 24
-            cron = "%s %s %s %s %s" % (min, hr, x, y, days)
-            click.echo("  %s" % pretty_cron.prettify_cron(cron))
+        click.echo("Only 'on' and 'off' are valid for timer")


And this one.

coveralls · 2017-09-16T15:45:15Z

Coverage decreased (-0.3%) to 32.784% when pulling b63b5d6 on vacuum_timer into 69ae780 on master.

rytilahti requested a review from syssi September 15, 2017 20:28

houndci-bot reviewed Sep 15, 2017

View reviewed changes

make hound happy

52c0c58

syssi approved these changes Sep 16, 2017

View reviewed changes

handle commentary from the pr

6398796

syssi reviewed Sep 16, 2017

View reviewed changes

be consistent about --enable and --disable

b63b5d6

syssi merged commit e3f6389 into master Sep 16, 2017

syssi deleted the vacuum_timer branch September 16, 2017 16:18

Uh oh!

Conversation

rytilahti commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rytilahti commented Sep 15, 2017

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!

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!

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

Uh oh!

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!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Sep 15, 2017 •

edited

Loading

coveralls commented Sep 15, 2017 •

edited

Loading

coveralls commented Sep 16, 2017 •

edited

Loading

coveralls commented Sep 16, 2017 •

edited

Loading