Home Original page

Add device fixture for D225(US) by alams154 · Pull Request #1596 · python-kasa/python-kasa

@alams154

Add D225 support to python-kasa.

@codecov

Codecov Report

❌ Patch coverage is 95.83333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.75%. Comparing base (f30f38b) to head (f17868d).

Files with missing lines Patch % Lines
kasa/smartcam/modules/battery.py 93.93% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1596      +/-   ##
==========================================
+ Coverage   92.73%   92.75%   +0.01%     
==========================================
  Files         157      157              
  Lines        9634     9670      +36     
  Branches      976      981       +5     
==========================================
+ Hits         8934     8969      +35     
- Misses        499      501       +2     
+ Partials      201      200       -1

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alams154

rytilahti

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! When adding new fixtures, please avoid doing changes elsewhere in the library if they are not very, very minor changes to adjust the library to make it work with the device in question. Instead, create a separate PRs, this will help to keep the git history clean.

@alams154

@alams154

Hi, thanks for the information. I've reverted the library changes. The fixture files can't be generated without some of those bigger library changes so I can open another PR for those. It throws "Unsupported device {discovery_result.ip} of type {type_} with no encryption type".

@rytilahti

I see, thanks for the clean up! Is it possible to do anything with the device before we get the other changes in? Just asking if this would be ready for merging after a review, or would it be better to wait for the follow up PR(s) before updating the readme?

If the only issue is the fixture generation, we could go and move forward to merge this already.

@alams154

I think we could proceed with merging the fixtures already.

There are still issues with fixture generation and running the kasa command without extra options (says "Unable to create device for <DEVICE_IP>") but I think we can address those in the next PR (#1602).

@rytilahti

Looks like the tests are failing, would it be possible to add to this PR only the minimal changes that make them pass?

@alams154

I don't think there is a minimal change since it affects discovery and then when that change is implemented, it causes more tests to fail regarding the battery which requires changes to the battery test.

What would be the best way to proceed now?

@alams154

Maybe we merge #1602 first and then merge this?

@rytilahti

Yes, that makes sense, I think. Would you mind taking care of cleaning up the PR to add only the necessary changes? Thanks!

@github-actions

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.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!