bencevans / node-sonos

🔈 Sonos Media Player Interface/Client
https://www.npmjs.com/package/sonos
MIT License
700 stars 146 forks source link

Sonos now ES6 class plus, two fixes and additional backporting #472

Closed pascalopitz closed 4 years ago

pascalopitz commented 4 years ago
hklages commented 4 years ago

SetBass and SetTreble values should be in range -10 to 10, integer.

pascalopitz commented 4 years ago

Yes, quite sure. Also spotted another copy paste mistake I made while trying to confirm to the coding style. I normally don't use comments unless it's an absolute gotcha, so am quite snow blind when it comes to docblocks. Apologies in both instances.

svrooij commented 4 years ago

I think this PR is getting pretty big, for us to be able to check it easily I would rather see in broken down in smaller pieces.

Because the first part doesn't need a lot of testing, but the second one would require a lot of manual testing (since we don't test every custom method defined there).

An other solution would be for us to release an alpha version to npm and let it be tested by various libraries using node-sonos and have it tested by use.

What do you guys think @bencevans @hklages

pascalopitz commented 4 years ago

Ok, I have broken this up into:

473 - a fix to my earlier contribution

474 - Some additional service methods on ContentDirectory, mainly to achieve triggering a share index

475 - Just a comment

476 - Event subscription and parsing to ContentDirectory, which enables us to listen to when a share indexing event has started/finished

477 - This is the rewrite of the main Sonos class to ES6, plus one additional method and two new service shorthands