-
-
Notifications
You must be signed in to change notification settings - Fork 223
Split out main cli module into lazily loaded submodules #1039
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1039 +/- ##
==========================================
+ Coverage 92.17% 92.22% +0.05%
==========================================
Files 85 95 +10
Lines 5928 6032 +104
Branches 1440 1453 +13
==========================================
+ Hits 5464 5563 +99
- Misses 358 363 +5
Partials 106 106 ☔ View full report in Codecov by Sentry. |
…to janitor/split_out_cli_next
kasa/cli/main.py
Outdated
lazy_subcommands={ | ||
"discover": "ka 10000 sa.cli.discover.discover", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nicer if we wouldn't need to have a static list but just add the commands here?
For grouped commands it should be really straightforward, i.e., just by adding the group to the main one, but individual commands would need to be imported and added manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention here is to avoid importing the commands up front and do it lazily to reduce the imports the cli does at startup: https://click.palletsprojects.com/en/8.1.x/complex/#lazily-loading-subcommands. I might not be following what you're suggesting though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I simplified this such that each submodule contains a group with the same name so only need to add the submodule name to the list. For the existing subcommands under device
and light
these are still made available at the parent cli command but we won't need to keep adding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our imports do not (or at least they should not) perform anything performance-intensive operations, so having them directly referenced will avoid potential issues if something gets renamed etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM & ready to be merged after you decide whether to keep main.py
or move its contents into the package __main__.py
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be some need to import this module externally? If not, maybe it's worth having its content directly inside the __main__.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if I'd done that on the first PR step it'd be ok but when I try it now it doesn't get picked up as a rename so we lose the history.
## [0.7.1](https://github.com/python-kasa/python-kasa/tree/0.7.1) (2024-07-31) [Full Changelog](0.7.0.5...0.7.1) **Release highlights:** - This release consists mainly of bugfixes and project improvements. - There is also new support for Tapo T100 motion sensors. - The CLI now supports child devices on all applicable commands. **Implemented enhancements:** - Expose reboot action [\#1073](#1073) (@rytilahti) - Decrypt KLAP data from PCAP files [\#1041](#1041) (@clstrickland) - Support child devices in all applicable cli commands [\#1020](#1020) (@sdb9696) **Fixed bugs:** - Fix iot light effect brightness [\#1092](#1092) (@sdb9696) - Enable setting brightness with color temp for smart devices [\#1091](#1091) (@sdb9696) - Do not send light\_on value to iot bulb set\_state [\#1090](#1090) (@sdb9696) - Allow erroring modules to recover [\#1080](#1080) (@sdb9696) - Raise KasaException on decryption errors [\#1078](#1078) (@sdb9696) - Update smart request parameter handling [\#1061](#1061) (@sdb9696) - Fix light preset module when list contains lighting effects [\#1048](#1048) (@sdb9696) - Handle module errors more robustly and add query params to light preset and transition [\#1036](#1036) (@sdb9696) - Fix credential hash to return None on empty credentials [\#1029](#1029) (@sdb9696) **Added support for devices:** - Add support for T100 motion sensor [\#1079](#1079) (@rytilahti) **Project maintenance:** - Bump project version to 0.7.0.5 [\#1087](#1087) (@sdb9696) - Fix generate\_supported pre commit to run in venv [\#1085](#1085) (@sdb9696) - Fix intermittently failing decryption error test [\#1082](#1082) (@sdb9696) - Fix mypy pre-commit hook on windows [\#1081](#1081) (@sdb9696) - Update RELEASING.md for patch releases [\#1076](#1076) (@sdb9696) - Use monotonic time for query timing [\#1070](#1070) (@sdb9696) - Fix parse\_pcap\_klap on windows and support default credentials [\#1068](#1068) (@sdb9696) - Add fixture file for KP405 fw 1.0.6 [\#1063](#1063) (@daleye) - Bump project version to 0.7.0.3 [\#1053](#1053) (@sdb9696) - Add KP400\(US\) v1.0.4 fixture [\#1051](#1051) (@gimpy88) - Add new HS220 kasa aes fixture [\#1050](#1050) (@sdb9696) - Add KS205\(US\) v1.1.0 fixture [\#1049](#1049) (@gimpy88) - Add KS200M\(US\) v1.0.11 fixture [\#1047](#1047) (@sdb9696) - Add KS225\(US\) v1.1.0 fixture [\#1046](#1046) (@sdb9696) - Split out main cli module into lazily loaded submodules [\#1039](#1039) (@sdb9696) - Structure cli into a package [\#1038](#1038) (@sdb9696) - Add KP400 v1.0.3 fixture [\#1037](#1037) (@gimpy88) - Add L920\(EU\) v1.1.3 fixture [\#1031](#1031) (@rytilahti) - Update changelog generator config [\#1030](#1030) (@sdb9696)
For ease of review there are no changes to existing logic within the cli commands themselves. The principal changes here are:
echo
command when--json
present.