python-snapcast icon indicating copy to clipboard operation
python-snapcast copied to clipboard

Published 20 hours ago • verified publisher icon happyleavesaoc

Initial support for Websockets connection to Snapcast Server

Open arpena opened this issue 2 years ago • 5 comments
trafficstars

Hi, I've added the support for Websockets to your wonderfully simple snapcast control interface. I'm currently using it on my not yet public snapvolctl project, a daemon to control a soundbar or other component using snapcast volume and playing updates. Hope you find useful.

arpena avatar Nov 17 '23 16:11 arpena

Hi, this is an interesting PR! Please add a few tests, and restore the setup.py named fields, and we should be good to merge. After that I'll cut and push a new release.

happyleavesaoc avatar Nov 19 '23 02:11 happyleavesaoc

Thanks ! It is working quite well in my environment and it is very stable. For the merge, I've restored the setup.py and corrected some pylint errors issued by tox on a newer commit. Should I create a new pull requests using that?

I'm not familiar with module testing on python, but from what I see in your tests, there is no connection to an actual server to test the client... so I believe that my modifications hadn't change the tests behavior. Is there some test that you have in mind that would test better the connection interaction?

arpena avatar Nov 24 '23 20:11 arpena

You can just push the changes to this branch and it should update this PR.

The tests mock the server (basically intercepting requests that would hit the server, and returning known data). It might be a pain to test websocket_handler that way, but SnapcastWebSocketProtocol should testable using the same pattern.

happyleavesaoc avatar Nov 24 '23 21:11 happyleavesaoc

Hi, thanks for catching up and providing tests. Could you please create a new PR from this? I have some small comments, but GitHub shows now all changes including those of already merged commits, which makes it hard to review. Next time maybe use rebase instead, keeps the history clean. (Here is a good explanation: https://developers.home-assistant.io/docs/development_catching_up) And just out of couriosity: what is the benefit of websockets compared the RPC?

luar123 avatar Apr 23 '24 20:04 luar123

Hi, thanks for catching up and providing tests. Could you please create a new PR from this? I have some small comments, but GitHub shows now all changes including those of already merged commits, which makes it hard to review. Next time maybe use rebase instead, keeps the history clean. (Here is a good explanation: https://developers.home-assistant.io/docs/development_catching_up) And just out of couriosity: what is the benefit of websockets compared the RPC?

I think there is no advantage of using websockets vs a raw tcp socket unless you are in the browser.
https://medium.com/kifi-engineering/websockets-vs-regular-sockets-b3b8e7ea0708

SantiagoSotoC avatar Jun 04 '24 00:06 SantiagoSotoC