declension / squeeze-alexa

Squeezebox integration for Amazon Alexa
GNU General Public License v3.0
59 stars 20 forks source link

Echo device dependent default players #121

Open stridger opened 5 years ago

stridger commented 5 years ago

Hi!

I'm not sure if you are interested in my patches here, but basically they do the following:

Thank you for this awesome project!

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 224


Changes Missing Coverage Covered Lines Changed/Added Lines %
squeezealexa/main.py 35 37 94.59%
squeezealexa/squeezebox/server.py 30 47 63.83%
<!-- Total: 66 85 77.65% -->
Totals Coverage Status
Change from base Build 211: -1.3%
Covered Lines: 1066
Relevant Lines: 1137

💛 - Coveralls
declension commented 5 years ago

Thanks for your interest in squeeze-alexa!

In general, yes, submissions are welcome, though please note the documented guidelines - especially around test coverage, and linking to pre-existing issues (or creating them first).

In terms of this PR, I'd strongly suggest splitting it up into its logical, independent PRs. Your bulleted list seems just right - one to fix #81, one to add utterances, and the slightly more controversial / involved one about stateful location and device-stickiness (which perhaps needs some more thought).

Logistically, I'd suggest creating the smallest PRs (and therefore easiest to get merged) first, and if / once merged, you can rebase / merge master back into this (which would become ) to reduce its size (Github will handle all this). Let me know if you run into trouble with this or would like some help.

stridger commented 5 years ago

I have already cleaned up all the tests earlier and that is now all passing by the looks of it. I'll look at increasing the coverage and doing more atomic PR later, I don't have much time to work on this, I'm afraid.

As you say, it's unclear if many people would be interested in sticky default players, but for me it's very important as I have an Echo Dot and a Squeezebox in every room.