search

svrooij / node-sonos-ts

:speaker: Sonos control library, use this library in your own appliction.
https://sonos-ts.svrooij.io/
MIT License
84 stars 18 forks source link

Parse information about stereo pairs from group state #98

Closed gauthierm closed 3 years ago

gauthierm commented 3 years ago

Adds information about stereo pairs to the parsed group information

After this change, the zone group is parsed as:

{ name: 'Mike's Office + 1',
  coordinator:
   { name: 'Mike's Office',
     uuid: 'RINCON_48A6B8A29E0401400',
     host: '192.168.1.21',
     port: 1400,
     Icon: 'x-rincon-roomicon:living',
     MicEnabled: false,
     SoftwareVersion: '60.3-81140',
     SwGen: '2',
     ChannelMapSet:
      { LF: 'RINCON_48A6B8A29E0401400',
        RF: 'RINCON_48A6B8A29E1801400' } },
  members:
   [ { name: 'Mike's Office',
       uuid: 'RINCON_48A6B8A29E1801400',
       host: '192.168.1.22',
       port: 1400,
       Icon: 'x-rincon-roomicon:living',
       MicEnabled: false,
       SoftwareVersion: '60.3-81140',
       SwGen: '2',
       ChannelMapSet: [Object] },
     { name: 'Mike's Office',
       uuid: 'RINCON_48A6B8A29E0401400',
       host: '192.168.1.21',
       port: 1400,
       Icon: 'x-rincon-roomicon:living',
       MicEnabled: false,
       SoftwareVersion: '60.3-81140',
       SwGen: '2',
       ChannelMapSet: [Object] } ] }
coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 451478445

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/services/zone-group-topology.service.ts 0 5 0.0%
<!-- Total: 0 5 0.0% -->
Totals Coverage Status
Change from base Build 426734931: -0.1%
Covered Lines: 856
Relevant Lines: 1421
💛 - Coveralls
svrooij commented 3 years ago

I’m all for parsing this information!!

But as stated in the top of this file, this file is generated and will be regenerated when the documentation changes.

your changes will be lost when I would regenerate the auto-generated part of this library. I moved the soap documentation, the generator and the template files to https://github.com/svrooij/sonos-api-docs/tree/main/generator/sonos-docs/templates/ts

So I will accept this PR (so you become a contributor) if you also make a PR for this file.

https://github.com/svrooij/sonos-api-docs/blob/4520afab0aa42eef0deb274ad84c5c241edcf6eb/generator/sonos-docs/templates/ts/service.hbs#L464-L527

gauthierm commented 3 years ago

Ah thanks. I didn't realize the file was generated. I will update the PR to move the change to the template. I'm considering unit tests for the new method. Where should tests live?

It seems like a really difficult way to develop the service to generate the TypeScript code with a template. You lose all type checking, linting, testing, code formatting, etc during development. Could the amount of templated code be reduced and all the custom methods and parsers moved to the main project in real TypeScript?

From an outside contributor's perspective, having all the separate service classes mashed into a single template makes it really hard to understand.

gauthierm commented 3 years ago

Opened https://github.com/svrooij/sonos-api-docs/pull/7/

svrooij commented 3 years ago

I totally agree that the service extensions are not really nice implemented, I'm open to suggestions.

I do like the fact that 90% of the code in those services is actually generated and that generating them from the service discovery costs only a few seconds.

I was thinking about creating the extensions in separate files in the services folder but that makes generating the index harder. Also the SonosDeviceBase is generated and should be changed to import the correct service.

svrooij commented 3 years ago

Sorry about the extra work! Since the extensions are now splitted to separate files I cannot merge this PR anymore. So will be closing it.

I've re-created your work here and would like to ask you to add your (anonymized) ZoneGroupState to the tests. See this comment