Extend schedule handling cli support #501
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #501 +/- ##
==========================================
- Coverage 79.59% 78.13% -1.47%
==========================================
Files 26 26
Lines 1960 2108 +148
Branches 602 673 +71
==========================================
+ Hits 1560 1647 +87
- Misses 359 419 +60
- Partials 41 42 +1
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for the PR @yparitcher! I added some initial review comments below.
| @@ -630,18 +631,113 @@ async def schedule(dev): | |||
|
|
|||
| @schedule.command(name="list") | |||
| @pass_dev | |||
| @click.option("--json", is_flag=True) | |||
There was a problem hiding this comment.
jsonifying is handled on the cli automatically for the return value, so there should be no need for special handling.
There was a problem hiding this comment.
This way i get the actual json that is passed to the device, not the python rule wrapped in json
$ kasa --host 192.168.0.xxx --type plug schedule list
id='B9A82B5BA5AB6F6DA4277429A226D632' name='7 Off 11 On' enable=<EnabledOption.Enabled: 1> wday=[1, 1, 1, 1, 1, 1, 1] repeat=<EnabledOption.Enabled: 1> sact=<Action.TurnOff: 0> stime_opt=<TimeOption.Enabled: 0> smin=420 eact=<Action.TurnOn: 1> etime_opt=<TimeOption.Enabled: 0> emin=660 s_light=None
$ kasa --host 192.168.0.xxx --type plug --json schedule list
id='B9A82B5BA5AB6F6DA4277429A226D632' name='7 Off 11 On' enable=<EnabledOption.Enabled: 1> wday=[1, 1, 1, 1, 1, 1, 1] repeat=<EnabledOption.Enabled: 1> sact=<Action.TurnOff: 0> stime_opt=<TimeOption.Enabled: 0> smin=420 eact=<Action.TurnOn: 1> etime_opt=<TimeOption.Enabled: 0> emin=660 s_light=None
[
"id='B9A82B5BA5AB6F6DA4277429A226D632' name='7 Off 11 On' enable=<EnabledOption.Enabled: 1> wday=[1, 1, 1, 1, 1, 1, 1] repeat=<EnabledOption.Enabled: 1> sact=<Action.TurnOff: 0> stime_opt=<TimeOption.Enabled: 0> smin=420 eact=<Action.TurnOn: 1> etime_opt=<TimeOption.Enabled: 0> emin=660 s_light=None"
]
$ kasa --host 192.168.0.xxx --type plug schedule list --json
{"id": "B9A82B5BA5AB6F6DA4277429A226D632", "name": "7 Off 11 On", "enable": 1, "wday": [1, 1, 1, 1, 1, 1, 1], "repeat": 1, "sact": 0, "stime_opt": 0, "smin": 420, "eact": 1, "etime_opt": 0, "emin": 660, "s_light": null}
There was a problem hiding this comment.
This needs to be fixed separately then (I think the proper way would be extending the json_formatter_cb by defining a serializer for pydantic BaseModel), but let's do that in a separate PR.
kasa/cli.py
Outdated
| @click.option("--wday", type=str, required=True) | ||
| @click.option("--sact", type=click.IntRange(-1, 2), default=None, required=True) | ||
| @click.option("--stime_opt", type=click.IntRange(-1, 2), default=None, required=True) | ||
| @click.option("--smin", type=click.IntRange(0, 1440), default=None, required=True) | ||
| @click.option("--eact", type=click.IntRange(-1, 2), default=-1) | ||
| @click.option("--etime_opt", type=click.IntRange(-1, 2), default=-1) | ||
| @click.option("--emin", type=click.IntRange(0, 1440), default=None) |
There was a problem hiding this comment.
I think we should modiy the Rule to have more user-friendly field names (using alias) and add some documentation on what input values they accept.
There was a problem hiding this comment.
I changed the names (If you better ones please comment), the doc still needs to be done.
Is there a way to pass the pydatic info to click? for example TimeOption
There was a problem hiding this comment.
I don't have a direct answer to that, sorry. I suppose generic support for all pydantic models is not that easy, but one can use manually inherit from click.ParamType to perform the conversion.
| if name is not None: | ||
| rule_to_edit.name = name | ||
| if enable is not None: | ||
| rule_to_edit.enable = 1 if enable else 0 |
There was a problem hiding this comment.
pydantic should handle the conversion just fine as long as the types are correct, so there should be no need for manual conversions here.
There was a problem hiding this comment.
I want that only the user specified options should override the existing rule fields, however if the user does not specify a flag we get a None value that i don't want to pass to pydantic to overwrite the field. If there is a better way to do this with click & pydantic please let me know. (I am not too familiar with either of them)
|
As i do not have bulbs, I did not implement bulb specific functionality, but i did leave the scaffolding for someone else to do it. |
|
I redid the rule inheritance a bit. not sure if there is a better way |
There was a problem hiding this comment.
I saw that you extend the time module to support timezone handling, could you please move that into its own PR to keep it easier to review? I'll try to find some time soon to take another look at this, alas, I won't be able to test this until some point next month.
| sact: Optional[Action] | ||
| stime_opt: TimeOption | ||
| smin: int | ||
| sact: Optional[Action] = Field(alias="start_action") |
There was a problem hiding this comment.
| sact: Optional[Action] = Field(alias="start_action") | |
| start_action: Optional[Action] = Field(alias="sact") |
This is wrong way around, no? Basically, the alias allows using a nicer name for the field name itself, reading (and serializing?) in this case from/to a json field called sact.
|
I don't have much time to work on this (it was a bit driveby, as it now work for me locally). But i hope to eventually split out the changes into separate PRs. Anyone is also welcome to fixup this PR if wanted |
Some improvements for the schedule command.
A draft for now, I plan on adding rule edit support, please give feedback
Fixes: #265
Fixes: #436
Based on the commands in the Thesis linked in that issue.
Tested on a EP10