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

Update docs for newer devices and DeviceConfig #614

Merged
merged 5 commits into from
Jan 10, 2024

Conversation

sdb9696
Copy link
Collaborator

@sdb9696 sdb9696 commented Jan 4, 2024

As per issue #600

Copy link

codecov bot commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (460054c) 85.23% compared to head (0e47cd4) 85.23%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #614   +/-   ##
=======================================
  Coverage   85.23%   85.23%           
=======================================
  Files          38       38           
  Lines        3475     3475           
  Branches      888      888           
=======================================
  Hits         2962     2962           
  Misses        432      432           
  Partials       81       81           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Looks much better, thanks 👍 just a minor maintenance nit that should be changed in all changed chunks if applicable.

docs/source/smartdevice.rst Outdated Show resolved Hide resolved
@rytilahti rytilahti added the documentation Improvements or additions to documentation label Jan 4, 2024
@rytilahti
Copy link
Member

Would you also mind adding somewhere a note that KASA_USERNAME and KASA_PASSWORD (or the hashed creds envvar) can be used instead of passing them always as command line arguments?

@sdb9696
Copy link
Collaborator Author

sdb9696 commented Jan 4, 2024

Would you also mind adding somewhere a note that KASA_USERNAME and KASA_PASSWORD (or the hashed creds envvar) can be used instead of passing them always as command line arguments?

Added to the cli.rst for KASA_USERNAME and KASA_PASSWORD. I didn't mention the hashed creds envvar because it 's more complicated there are three different potentail hash values, one for AES, one for Klap and one for Klap V2.

@sdb9696 sdb9696 requested a review from rytilahti January 4, 2024 17:44
docs/source/cli.rst Outdated Show resolved Hide resolved
@sdb9696 sdb9696 force-pushed the update_docs branch 2 times, most recently from a951483 to 296c11c Compare January 4, 2024 18:51
@sdb9696 sdb9696 requested a review from rytilahti January 4, 2024 18:51
@rytilahti
Copy link
Member

rytilahti commented Jan 4, 2024

https://python-kasa--614.org.readthedocs.build/en/614/discover.html should be modified to contain at least some basic description about how the discovery works and how it should be used. Doesn't need to be anything fancy, but a quick description and a copy-pasteable code example that can be run as-is would be great.

Same would also be great in the smartdevice/common api, i.e., just a very simple ready-to-use example on how to get started if you have never used the lib.

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.

If no command is given, the state command will be executed to query the device state.

The "Command-line usage" section in cli.rst should also be adapted to describe briefly the options related to authentication, no need to go deep into details but giving enough information get get one started from zero.

docs/source/cli.rst Outdated Show resolved Hide resolved
@sdb9696
Copy link
Collaborator Author

sdb9696 commented Jan 5, 2024

All comments now addressed.

@sdb9696 sdb9696 requested a review from rytilahti January 5, 2024 07:56
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.

This looks great, thanks! 🥇

I added a couple of very minor nits in places where I think rewording makes sense, feel free to adjust or keep it as it is, your call.

docs/source/cli.rst Outdated Show resolved Hide resolved
docs/source/cli.rst Outdated Show resolved Hide resolved
docs/source/discover.rst Outdated Show resolved Hide resolved
docs/source/discover.rst Outdated Show resolved Hide resolved
docs/source/discover.rst Outdated Show resolved Hide resolved
docs/source/smartdevice.rst Outdated Show resolved Hide resolved
docs/source/smartdevice.rst Outdated Show resolved Hide resolved
docs/source/smartdevice.rst Outdated Show resolved Hide resolved
kasa/discover.py Show resolved Hide resolved
kasa/discover.py Outdated Show resolved Hide resolved
@sdb9696
Copy link
Collaborator Author

sdb9696 commented Jan 10, 2024

All comments now addressed

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, thanks! 👍

@rytilahti rytilahti merged commit 3e0cd07 into python-kasa:master Jan 10, 2024
29 checks passed
@sdb9696 sdb9696 deleted the update_docs branch February 20, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants