-
Notifications
You must be signed in to change notification settings - Fork 33
Initial support for Websockets connection to Snapcast Server #60
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
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.
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?
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.
Synchronize on unknown stream update and ignore input-only streams
Fix group friendly name
Fix connection related issues
Implement add and remove streams and path property
snapcast server
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?
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
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.