8000 Split out main cli module into lazily loaded submodules by sdb9696 · Pull Request #1039 · python-kasa/python-kasa · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 9 commits into from
Jul 23, 2024

Conversation

sdb9696
Copy link
Collaborator
@sdb9696 sdb9696 commented Jul 3, 2024

For ease of review there are no changes to existing logic within the cli commands themselves. The principal changes here are:

  • The moving to separate modules
  • The lazy loading of those modules
  • Change of the (probably flaky) logic to supress the echo command when --json present.

Copy link
codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 81.68449% with 137 lines in your changes missing coverage. Please review.

Project coverage is 92.22%. Comparing base (06ff598) to head (ac09112).
Report is 80 commits behind head on master.

Files with missing lines Patch % Lines
kasa/cli/light.py 64.39% 46 Missing and 1 partial ⚠️
kasa/cli/usage.py 70.73% 23 Missing and 1 partial ⚠️
kasa/cli/discover.py 83.15% 14 Missing and 2 partials ⚠️
kasa/cli/schedule.py 56.00% 11 Missing ⚠️
kasa/cli/main.py 72.97% 10 Missing ⚠️
kasa/cli/feature.py 85.71% 6 Missing and 2 partials ⚠️
kasa/cli/device.py 93.96% 6 Missing and 1 partial ⚠️
kasa/cli/lazygroup.py 81.08% 5 Missing and 2 partials ⚠️
kasa/cli/time.py 89.65% 2 Missing and 1 partial ⚠️
kasa/cli/__main__.py 0.00% 2 Missing ⚠️
... and 1 more
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.
📢 Have feedback on the report? Share it here.

@sdb9696 sdb9696 added this to the 0.7.1 milestone Jul 4, 2024
@sdb9696 sdb9696 added the maintenance Project improvements and maintenance label Jul 4, 2024
Base automatically changed from janitor/split_out_cli to master July 4, 2024 12:52
kasa/cli/main.py Outdated
Comment on lines 64 to 65
lazy_subcommands={
"discover": "ka 10000 sa.cli.discover.discover",
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Member

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.

< 10000 div class="pr-1 flex-auto min-width-0" > Use dict in _legacy_type_to_class
Copy link
Member
@rytilahti rytilahti left a 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!

Copy link
Member

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?

Copy link
Collaborator Author

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.

@sdb9696 sdb9696 merged commit ed03367 into master Jul 23, 2024
24 of 25 checks passed
@sdb9696 sdb9696 deleted the janitor/split_out_cli_next branch July 23, 2024 18:13
@sdb9696 sdb9696 mentioned this pull request Jul 31, 2024
sdb9696 added a commit that referenced this pull request Jul 31, 2024
## [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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Project improvements and maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0