-
Notifications
You must be signed in to change notification settings - Fork 52
feat: add a sound volume trait #477
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
Conversation
| response = await self._rpc_channel.send_command(RoborockCommand.GET_SOUND_VOLUME) | ||
| if isinstance(response, list) and response: | ||
| return int(response[0]) |
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.
I think this is something we should definitely try to figure out in the future system. I don't think we should always have to check if it is a list, I think you said it is just because you changed the typing for send_command
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.
Does the API always return a list? My impression was that it does not given some of the other code. If it does always return a list, then that's good to know.
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.
I honestly can't remember. I think we may always wrap it in a list? We didn't have to do this explicit check before so I can't remember what we did
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.
I think this always returns a list with a number in it...
I had to take out the code that always pops the data structure out of the list for the clean summary https://github.com/Python-roborock/python-roborock/pull/476/files otherwise it only returns one number from the list.
allenporter
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.
I ended up rebasing and merging, then adding a test with the json for the sound response just to get that source of truth checked in.
|
Fixed lint errors, need one more approval, sorry about that. |
Add a volume trait. The purpose is to have more trait use cases to figure out a better trait API.
This improves error response handling, found during adding the volume trait and making some mistakes.
This will have merge conflicts with #476 and will likely need another pass of review.