-
-
Notifications
You must be signed in to change notification settings - Fork 221
Enable multiple requests in smartprotocol #584
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
Enable multiple requests in smartprotocol #584
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #584 +/- ##
==========================================
+ Coverage 83.63% 84.78% +1.15%
==========================================
Files 37 37
Lines 3183 3220 +37
Branches 803 810 +7
==========================================
+ Hits 2662 2730 +68
+ Misses 436 413 -23
+ Partials 85 77 -8 ☔ View full report in Codecov by Sentry. |
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.
Nice work, seems to be (mostly) working! 🥇 I added a couple of comments to avoid unnecessary deep nesting, which should help to keep it easier to test and read.
The device main query succeeds querying multiple "modules", but looks like there is a regression on emeter access:
File "/home/tpr/code/python-kasa/kasa/cli.py", line 474, in state
echo(f"\t{emeter_status}")
^^^^^^^^^^^^^^^^^^^^
File "/home/tpr/code/python-kasa/kasa/emeterstatus.py", line 52, in __repr__
f"<EmeterStatus power={self.power} voltage={self.voltage}"
^^^^^^^^^^
File "/home/tpr/code/python-kasa/kasa/emeterstatus.py", line 30, in power
return self["power"]
~~~~^^^^^^^^^
File "/home/tpr/code/python-kasa/kasa/emeterstatus.py", line 83, in __getitem__
return self.__getitem__(i) / 1000
~~~~~~~~~~~~~~~~~~~~^~~~~~
TypeError: unsupported operand type(s) for /: 'NoneType' and 'int'
The emeter status container contains only dict_items([('power_mw', None), ('total', None)])
.
All comments addressed, emeter regression is fixed and the test framework tweaked to cover this regression case. |
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.
Looks good to me, I'd extract the error code passing into its own PR though 👍
Hi @rytilahti, that's extracted now. I'll submit the new PR once this is merged and I can rebase on top. |
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.
Looks nice and tidy now, thanks!
Hopefully a nice and small PR to enable
multipleRequest
insmartprotocol
and the test framework.A subsequent PRs will introduce the smart request helper functions and use the
component_nego
result to determine which requests to make rather than hardcoding.