Added ability to play by Album or Artist (US only) and updated Genre to use Amazon Slot Type

declension / squeeze-alexa

Squeezebox integration for Amazon Alexa

GNU General Public License v3.0

59 stars 20 forks source link

Added ability to play by Album or Artist (US only) and updated Genre to use Amazon Slot Type #77

Closed NadirJ closed 5 years ago

NadirJ commented 5 years ago

Fixes#10

US-Only: I added the ability to play by album and artist using Amazon Slot Types: AMAZON.MusicAlbum and AMAZON.MusicGroup

Other locales: I also updated the play genre intent to use the AMAZON.Genre intent and tested it for US_EN

declension commented 5 years ago

Cool, looks interesting. Could we split this PR though - some of the stuff looks like simple / universal changes (e.g. Genre - I'll make an issue, meant to do this a while ago actually) but the Album / Artist stuff needs careful though when it comes to other regions, plus I want to try and keep the invocation language similar to native / other audio VUIs.

NadirJ commented 5 years ago

I'll break Genres out its a simple intent change (no python actually changed)

For the album/artist the python side is generic. However the intent json is language/region specific.

For regions where the slot types AMAZON.MusicGroup AMAZON.MusicAlbum are not supported you would have to create your own slot types for now (which is painful but doable)

NadirJ commented 5 years ago

I just tried removing the Album and Artist Intents and leaving the new python methods in place and nothing breaks. So I think this will be safe to put this in I can update the region specific intents when Amazon supports more regions

declension commented 5 years ago

OK, right can we:

remove the GENRE bits from this one (or I can push these to master separately if you prefer)
Likewise with the v1 genre test - that's good stuff but doesn't feel related to this.
and most importantly make sure there are tests for all the new areas of code (I've enabled building of forks in the CI so I think all PRs should now be run through, but let me know if they're not). Have a look at the Coveralls project page if you haven't seen it yet

declension commented 5 years ago

Oh and I guess copying the en_US to an en_GB intents.json is something I'll probably do to make sure people on non-US v1 still have something to refer to

NadirJ commented 5 years ago

Everything you said above makes sense.

I'll pull out the GENRE stuff in to a separate PR
I have run the tests locally including the new intent test but I'll make sure CI works

I'll update the PR in a couple of days