-
-
Notifications
You must be signed in to change notification settings - Fork 232
Add device fixture for D225(US) #1596
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
rytilahti
left a comment
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.
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.
|
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". |
|
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. |
|
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). |
|
Looks like the tests are failing, would it be possible to add to this PR only the minimal changes that make them pass? |
|
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? |
|
Maybe we merge #1602 first and then merge this? |
|
Yes, that makes sense, I think. Would you mind taking care of cleaning up the PR to add only the necessary changes? Thanks! |
Add D225 support to python-kasa.