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

Unified and scalable command line interface #191

Merged
merged 16 commits into from
Mar 31, 2018
Merged

Conversation

yawor
Copy link
Contributor

@yawor yawor commented Jan 28, 2018

I think that the current approach to command line interface, where different types of devices have separate commands has a lot of code duplication and is not very scalable. Right now only four device classes are even directly supported. Making more classes supported would mean more *_cli.py files making even harder to maintain.

Here's my proposition. I've created a single entry point in miio.cli, which is registered as miio command during setup. I've played around with some metaclass programming and click to create sub-commands auto-registration mechanism. I've added glue logic to the AirPurifier and Vacuum classes (Vacuum is WIP).

Main elements are the DeviceGroupMeta meta class which needs to be added to the specific device class and device_command decorator, which itself takes a decorators collection in form of *args. This is something I'm calling a lazy decorator application (couldn't find a better name for it :)). The decorators in the collection are not applied and no click commands are created unless actually used from miio.cli.
This means that the classes can still be used directly by other software without any changes but the classes are augmented when used through the new command line miio.

An example usage:
miio airpurifier --ip <some ip> --token <some token> status
miio airpurifier --ip <some ip> --token <some token> set_mode auto
miio vacuum --ip <some ip> --token <some token> status
etc

I've also enabled auto envvar prefix in Click, so the ip and token can be set for each device class like this:

export MIIO_AIRPURIFIER_IP=1.2.3.4
export MIIO_AIRPURIFIER_TOKEN=asdf.....
export MIIO_VACUUM_IP=5.6.7.8
export MIIO_VACUUM_TOKEN=asdf.....

There's also an example of Group customisation in Vacuum class in the form of get_device_group class method. When that method is present, it's responsible for producing a command group. In the example I've re-implemented the sequence file loading and saving.

Please don't hesitate to comment, criticise etc. It's still WIP so I'm open to suggestions.

@coveralls
Copy link

coveralls commented Jan 28, 2018

Coverage Status

Coverage decreased (-1.4%) to 69.884% when pulling 1b66cce on yawor:cli into 2d44ede on rytilahti:master.

@@ -11,6 +11,9 @@
import ipaddress
import miio
import logging
from typing import Union
from functools import wraps
from functools import partial

Choose a reason for hiding this comment

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

module level import not at top of file

@@ -11,6 +11,9 @@
import ipaddress
import miio
import logging
from typing import Union
from functools import wraps

Choose a reason for hiding this comment

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

module level import not at top of file

@@ -11,6 +11,9 @@
import ipaddress
import miio
import logging
from typing import Union

Choose a reason for hiding this comment

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

module level import not at top of file

@@ -3,7 +3,10 @@
import re
from typing import Any, Dict, Optional
from collections import defaultdict
from functools import wraps

Choose a reason for hiding this comment

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

'functools.wraps' imported but unused

@yawor
Copy link
Contributor Author

yawor commented Jan 30, 2018

@rytilahti @syssi could you take a look? Do you see any issues with that approach? I can start porting current command line features to use new approach, but it requires some effort to do that, so it would be nice to know if you like it. If not then I'll drop it.

@rytilahti
Copy link
Owner

rytilahti commented Jan 30, 2018

First of all I applaud that you have done this in a such short time (I have had this on my pipeline for a while with a similar metaclass approach, but never got into the semantics to make it really work), this is really awesome piece of work! In my earlier experiments I didn't want to introduce 'click' into the library parts, but I think this is unavoidable for the time being and not worth tinkering.

Unfortunately I haven't had much spare time lately, so I must apologize for not answering your great write-up wrt async support. However, as I have wanted such a generalized cli for a longer time, I played around with this PR a bit (and implemented inheriting commands from base-classes, and added support for the plug, feel free to cherry-pick and modify as needed from https://github.com/rytilahti/python-miio/commits/common_cli_improvements), and all in all it feels good but could use some polisihing.

Some discussion points:

  1. miio is already used by miio (javascript library), so I'm not sure if we want deliberately cause a naming clash (and potential confusion caused by that). However at the moment I have no better name to propose, 'pymiio' sounds somewhat lame.

  2. Commands missing a docstring should return an easily recognizable placeholder:
    log_upload_status partial(func, *args, **keywords) - new...

  3. We need to have a way to define a default "action" when no command is specific, much to how 'status' gets currently called per default, but maybe this can differ between devices?

  4. In case a boolean value is returned by the command, but no result handling is defined, I think it'd be nice to have some sort of basic "Success" or "Failure" messaging.

  5. Having device_command() should be alone enough to return some user-readable output, e.g. by just echoing the result from the call. Would command() be a better name for the decorator, btw?

  6. --ip and --token should be on the top level, not after the "type" parameter.

  7. I'm not completely happy with the string templates, looks like this is especially with airpurifier a really huge, although I have no proposals how to make this better for now. Other than that, I think it's better not to use multiline comments, but format the string manually (see how I did it for the plug).

About configuration handling:

  1. What has been for a longer time on my todo is a global config handling: instead of passing ip & token, a config object containing relevant information (such as IP address, token, model and/or class implementing support) should be allowed, or a factory method for creating device classes based on those objects should be provided.

This could be used to provide an interface such as miio --device myrobot on without having to pass token and ip separately, as they are not very nice when you have a lots of devices (sometimes even from of the same type).

I have some code creating simple config files based on backup files, but never got that far enough with my common interface hack to integrate it in. Using the model information would also pave a way for having model-specific interfaces (simply by overloading the commands on subclasses of the main one), if wanted :-)

edit: one last thing I wanted to mention is, that there is also a need for generic cache file handling, as all of the devices (most likely) will require saving the sequence number. Or at least it would be a sane approach to take on that topic. So instead of having that handling inside the Vacuum class, it should be done for every type of supported devices. The vacuum one just extends that handling by having an extra sequence id for the manual control, maybe other devices could use some sort of extra state saving, too?

edit2: this is really the last update, but I think it may be worth considering how easily can we extend this to support other types of output formats (see #98).

@yawor
Copy link
Contributor Author

yawor commented Jan 30, 2018

@rytilahti thanks for the comments. For now I'll only address some points. I'll be away few days (urgent family matters) and then I'll try to go through it thoroughly.

  1. I've also had a problem naming the command. I've used miio for now as this is more like a proof of concept/work in progress right now. I've been thinking about "mi".
  2. It could be done even now by using get_device_group class method and assigning a custom callback to the group together with setting invoke_without_command. But I think it can be done better. For example a bool keyword arg default for the device_command decorator with some checks in the metaclass. It would only work for default device group callback, so when customising one that option would not work (it would still have to be done in the custom callback).
    4, 5. Regarding result display, I've implemented it right now using a decorator. I've only implemented this single echo_return_status for now as an example how it could work. I think more correct name for it would be echo_formatted_result_status or something like that. As you can see, as an added bonus it also supports callables. The problem with this approach is that there's no option now to have a default result processing if none is provided for the command. It would require marking somehow the decorators meant for echoing the results and then check for them in the list of the attached decorators.
    Some solutions:
  • don't use decorators for echoing results, instead set a callable as a keyword arg for device_command decorator
  • add more echoing decorators for some simple results
    BTW the echo_return_status works in two steps. First it displays a message, then calls the method and displays the result.
  1. No problem, it can be renamed to command.
  2. Why's that? It complicates other commands in main group as it would require to do checks in the group callback like it's done now in current implementation. IP and token are required for a device so it was natural for me that they should be provided at that level. Right now you can attach other commands and groups to main cli group without worrying about unnecessary arguments and options.
  3. It could be done with custom echo decorators/callables. The status result formatting for airpurifier could be done in the AirPurifierStatus as a method which then could be called from lambda given to echo_return_status. There are many different ways this can be done.

@yawor
Copy link
Contributor Author

yawor commented Feb 4, 2018

@rytilahti regarding your edits:

  • Edit 1: I don't know how devices other than the air purifiers behave but at least in case of these I didn't see any issues with sequence numbers being repeated all the time. When testing my changes I often send a single command with id = 1 and there's no problem at all. Every request is accepted (if the method and arguments are correct of course). From what I've seen the sequence number is more like a way to correlate request and response, as the response always has the same id/sequence as the request. I've been playing around with my asyncio implementation and I'm sending multiple requests at once (on the same UDP port, I don't close and reopen the UDP socket for each request) and then I'm using the id/sequence in the response to correctly return the result for each call.
  • Edit 2: I agree. But it may complicate the output configuration a bit. I need to rethink this part.

@yawor
Copy link
Contributor Author

yawor commented Feb 5, 2018

@rytilahti what configuration format do you have in mind? Would you want to stick to something simple like built-in ini-file parser? Or maybe something more advanced like yaml?
If we could go with yaml, then I think it would also be good to add Voluptuous to dependencies to validate the configuration structure.

An example yaml config structure:

devices:
  ap1:
    class: AirPurifier
    ip: 1.2.3.4
    token: asdf....

The devices is a dict, to make sure that the device names are unique.
The main cli group could try to read it from one of the default locations or from location specified by the option on command line and generate additional subgroups for each device defined in the config.
Then miio ap1 status would use data from config. Some namespace separation may be needed though, in case a user defines a name which would shadow the name of the device class. So maybe it could work like this:

  • to call a device class without config: miio <type of the device> ... (no changes here)
  • to call a device from config: miio device <device name from config> ...
    Also that way may be easier to implement using a custom group class which would read the config file when needed.

@rytilahti
Copy link
Owner

  1. I've also had a problem naming the command. I've used miio for now as this is more like a proof of concept/work in progress right now. I've been thinking about "mi".

I really like 'mi', unfortunately it seems to be used by this: https://gentoo.com/di/ :(

  1. It could be done even now by using get_device_group class method and assigning a custom callback to the group together with setting invoke_without_command. But I think it can be done better. For example a bool keyword arg default for the device_command decorator with some checks in the metaclass. It would only work for default device group callback, so when customising one that option would not work (it would still have to be done in the custom callback).

Just wanted to point it out, at the moment I have no solution but you seem to be knowledgeable in the matter so I'll leave it to you for now :-)

4, 5. Regarding result display, I've implemented it right now using a decorator. <snip>
Some solutions:
  • don't use decorators for echoing results, instead set a callable as a keyword arg for device_command decorator

I think this sounds the sanest, it'll also be fairly simple to provide a default implementation just for echoing the result I suppose.

  • add more echoing decorators for some simple results

This can be done by having default handlers for error and non-error cases, I think.

  1. Why's that? It complicates other commands in main group as it would require to do checks in the group callback like it's done now in current implementation. IP and token are required for a device so it was natural for me that they should be provided at that level. Right now you can attach other commands and groups to main cli group without worrying about unnecessary arguments and options.

I somehow thought that those are "global" options, and should therefore belong to the top level. However after using your cli and implementing more devices to use it, I'm starting to feel that the way it's currently done is just fine.

  1. It could be done with custom echo decorators/callables. The status result formatting for airpurifier could be done in the AirPurifierStatus as a method which then could be called from lambda given to echo_return_status. There are many different ways this can be done.

See the above comment wrt. echo handlers. Adding a pretty_printable, or maybe just a dict of (variable, description, postfix/measurement_unit) elements could do it? This would also allow easy JSONification of those.

Edit 1. I don't know how devices other than the air purifiers behave but at least in case of these I didn't see any issues with sequence numbers being repeated all the time. When testing my changes I often send a single command with id = 1 and there's no problem at all. Every request is accepted (if the method and arguments are correct of course). From what I've seen the sequence number is more like a way to correlate request and response, as the response always has the same id/sequence as the request. I've been playing around with my asyncio implementation and I'm sending multiple requests at once (on the same UDP port, I don't close and reopen the UDP socket for each request) and then I'm using the id/sequence in the response to correctly return the result for each call.

They updated the vacuum's protocol at some point and made it much stricter, that's why the library bumps the sequence number of failures. I don't know if that's the case for rest of the devices, but having a way to map requests and responses can at least be useful with your asynchronous implementation later on.

Edit 2: I agree. But it may complicate the output configuration a bit. I need to rethink this part.

In some cases (such as for status), I think it'd be enough just to have a way to get the raw presentation of the container as a dict.

@rytilahti
Copy link
Owner

what configuration format do you have in mind? Would you want to stick to something simple like built-in ini-file parser? Or maybe something more advanced like yaml?

I prefer to avoid the built-in ini-file parser for a reason or another, but I don't really mind that much. Back in the days I was doing some tests with anyconfig based on backups with this simple create_config script:

import click
from miio.extract_tokens import BackupDatabaseReader
import anyconfig
import attr

@click.command()
def main():
    b = BackupDatabaseReader()
    devices = [attr.asdict(x) for x in b.read_tokens("backup/miio2.db")]

    config = {'devices': {x['name']: x for x in devices}}
    anyconfig.dump(config, "/tmp/test.yml", default_flow_style=False)

if __name__ == "__main__":
    main()

which produces a yaml (and can be easily changed to produce inis or jsons, too) similar to this:

devices:
  Flower Care:
    name: Flower Care
    mac: C4:7C:8D:XX:XX:XX
    ip: 134.147.XXX.XXX
    token: 124f90d87b4b906XXX
    model: hhcc.plantmonitor.v1
  Mi Robot Vacuum:
    name: Mi Robot Vacuum
    mac: 28:6C:07:XX:XX:XX
    ip: 192.168.XXX.XXX
    token: 476e6b70343055483230644c53XXX
    model: rockrobo.vacuum.v1

From the looks of it it seems that we think alike :-) I was merely adding the models instead of class names, as we have a mapping for this available (which we could move out from discovery to be more easily accessible).

  • to call a device class without config: miio ... (no changes here)
  • to call a device from config: miio device ...
    Also that way may be easier to implement using a custom group class which would read the config file when needed.

This sounds like the winning solution to me!

The location of the file can be reached with appdirs on which we depend on already, to make it OS independent.

miio/plug.py Outdated
@command(
default_output=format_output("",
"Power: {result.power}\n"
"Temperature: {result.temperature}")

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

miio/plug.py Outdated

class Plug(Device):
"""Main class representing the smart wifi socket / plug."""

@command(
default_output=format_output("",
"Power: {result.power}\n"

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent


def json_output(pretty=False):
indent = 2 if pretty else None
def decorator(func):

Choose a reason for hiding this comment

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

expected 1 blank line before a nested definition, found 0

from typing import Union
from functools import wraps
from functools import partial
from .exceptions import DeviceError

Choose a reason for hiding this comment

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

module level import not at top of file

import json
from typing import Union
from functools import wraps
from functools import partial

Choose a reason for hiding this comment

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

module level import not at top of file

@@ -11,6 +12,11 @@
import ipaddress
import miio
import logging
import json
from typing import Union
from functools import wraps

Choose a reason for hiding this comment

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

module level import not at top of file

@@ -11,6 +12,11 @@
import ipaddress
import miio
import logging
import json
from typing import Union

Choose a reason for hiding this comment

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

module level import not at top of file

@@ -11,6 +12,11 @@
import ipaddress
import miio
import logging
import json

Choose a reason for hiding this comment

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

module level import not at top of file

@yawor
Copy link
Contributor Author

yawor commented Feb 12, 2018

I've changed how the output decorator is applied to the command decorator. It now needs to be passed as a default_output keyword arg. If it's not assigned, then the default is created during the call which outputs a message: "Running command {command_name}" and displays the result without any formatting.

I've also implemented an output option for top level cli group and created an output attribute in the GlobalContextObject class, which can pass any decorator which will override the default one. As an example I've implemented the json_output decorator and added output option to top level cli group which allows switching to json or json_pretty output.

@rytilahti
Copy link
Owner

Btw, not all dicts are json serializable, but I suppose we are not going to have structures which are not, so I think it's okay. Do you think we could start moving towards merging this to master (after the next bugfix release) and fix the issues as they come?

We can either keep the existing tools in place, or create simplified wrappers to keep the interfaces intact. I'd prefer the latter as that'd allow us to clean up the code-base.

@yawor
Copy link
Contributor Author

yawor commented Feb 17, 2018

The json_output decorator can be enhanced by creating a class inheriting from json.JSONEncoder, which would deal with data other than what json supports by default.

I need to do some tests to check if this approach can be mixed with asyncio, to be sure that this whole PR won't block switching to async.

@rytilahti
Copy link
Owner

Wrt JSONEncoder, I just mentioned it as the dunder is called __json__() although it returns back a dict (or even a list in some cases?). The general approach is sound anyway.

@yawor
Copy link
Contributor Author

yawor commented Feb 17, 2018

Yeah, it's a naming issue. Maybe it should be called __jsondata__. I want to have the json.dumps` in one place, so it can be controlled, so any object not supported by json needs some way to return its json serializable data. The data can be anything that is supported by json.

@rytilahti
Copy link
Owner

Maybe as_dict, or __dict__ just to be clear that it's merely a dict ready to be converted? Otherwise I agree that it's all good as soon as you give it a 'yes'.

from typing import Union
from functools import wraps
from functools import partial
from .exceptions import DeviceError

Choose a reason for hiding this comment

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

module level import not at top of file

import re
from typing import Union
from functools import wraps
from functools import partial

Choose a reason for hiding this comment

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

module level import not at top of file

import json
import re
from typing import Union
from functools import wraps

Choose a reason for hiding this comment

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

module level import not at top of file

@yawor
Copy link
Contributor Author

yawor commented Mar 10, 2018

Sorry for my recent absence. I've rebased the code on the current master branch and I've added better support for enum args (based on the code posted in pallets/click#605). The new EnumType is used in AirPurifier class.
I also did some tests with the asyncio and this approach won't block the migration to async in the future. So I think we can start moving the cli to this unified approach and iron out any issues along the way.

What about the command name?

@rytilahti
Copy link
Owner

Not a thing, welcome back! The name question is quite tricky, mirobo was/is pretty good as it allows easy googling compared to mi. miio would obviously be the best choice, but I'm worried about name-clashing with the javascript project.. :-(

@syssi
Copy link
Collaborator

syssi commented Mar 10, 2018

What about miiocli or miioctl?

@rytilahti
Copy link
Owner

rytilahti commented Mar 10, 2018

👍 for miiocli, we shall still keep the existing mi* tools around with a deprecation warning for some time. I think this PR can be merged after we get 0.3.8 released, if @yawor says it's fine. The title should be updated to be something more descriptive for the changelogs before the merge.

@syssi
Copy link
Collaborator

syssi commented Mar 31, 2018

@yawor did you see the last comment? I would like to merge the feature as soon as possible.

@syssi syssi changed the title WIP: Trying a different approach to command line interface Unified command line interface Mar 31, 2018
miio/plug.py Outdated
@command(
default_output=format_output("",
"Power: {result.power}\n"
"Temperature: {result.temperature}")

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

miio/plug.py Outdated

@command(
default_output=format_output("",
"Power: {result.power}\n"

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

miio/plug.py Outdated
@@ -0,0 +1,76 @@
import logging
from typing import Dict, Any, Optional

Choose a reason for hiding this comment

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

'typing.Optional' imported but unused

def usb_on(self):
"""Power on."""
return self.send("set_usb_on", [])

@command(
default_output = format_output("Powering USB off"),

Choose a reason for hiding this comment

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

unexpected spaces around keyword / parameter equals

def off(self):
"""Power off."""
if self.model == MODEL_CHUANGMI_PLUG_V1:
return self.send("set_off", [])

return self.send("set_power", ["off"])

@command(
default_output = format_output("Powering USB on"),

Choose a reason for hiding this comment

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

unexpected spaces around keyword / parameter equals

def on(self):
"""Power on."""
if self.model == MODEL_CHUANGMI_PLUG_V1:
return self.send("set_on", [])

return self.send("set_power", ["on"])

@command(
default_output = format_output("Powering off"),

Choose a reason for hiding this comment

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

unexpected spaces around keyword / parameter equals

@@ -123,28 +137,47 @@ def status(self) -> ChuangmiPlugStatus:
return ChuangmiPlugStatus(
defaultdict(lambda: None, zip(properties, values)))

@command(
default_output = format_output("Powering on"),

Choose a reason for hiding this comment

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

unexpected spaces around keyword / parameter equals

@syssi syssi changed the title Unified command line interface Unified and scalable command line interface Mar 31, 2018
@syssi syssi merged commit 5973100 into rytilahti:master Mar 31, 2018
@yawor yawor deleted the cli branch April 13, 2018 13:03
@yawor
Copy link
Contributor Author

yawor commented Apr 13, 2018

Hi guys. I'm sorry for my absence. It's awesome seeing this PR merged already and also extended by adding support in other device types :). I hope I'll be able to get back to help making this library better soon. Asyncio support waits for some love :D.

@rytilahti
Copy link
Owner

rytilahti commented Apr 15, 2018

Thank you @yawor for making this happen in the first place & you @syssi for adapting the other existing devices! 💯 🥇

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

Successfully merging this pull request may close these issues.

5 participants