Repository navigation
Extend schedule handling cli support - #501
yparitcher wants to merge 17 commits into
Conversation
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 72.97297% with 40 lines in your changes missing coverage. Please review.
@@ 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 Harness.
|
Sorry, something went wrong.
rytilahti
left a comment
There was a problem hiding this comment.
Thanks for the PR @yparitcher! I added some initial review comments below.
Sorry, something went wrong.
|
|
||
| @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.
Sorry, something went wrong.
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}
Sorry, something went wrong.
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.
Sorry, something went wrong.
| @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.
Sorry, something went wrong.
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
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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)
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I redid the rule inheritance a bit. not sure if there is a better way |
Sorry, something went wrong.
rytilahti
left a comment
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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Sorry, something went wrong.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Sorry, something went wrong.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Sorry, something went wrong.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Sorry, something went wrong.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Sorry, something went wrong.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Sorry, something went wrong.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Sorry, something went wrong.
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