search

jimmywarting / wemo-client

JavaScript client library for controlling and subscribing to Wemo Devices
MIT License
132 stars 40 forks source link

discover method should return the current state of all devices #34

Closed richardwillars closed 8 years ago

richardwillars commented 8 years ago

With the current library there is no way to know when WeMo devices have been removed from the local network. Currently the discover method only returned newly discovered devices.

This change ensures that all devices returned represent the current state of the WeMo devices on the network. Not only is this more accurate but it allows the consumer to understand when devices have been removed from the network.

codecov-io commented 8 years ago

Current coverage is 89.96% (diff: 100%)

Merging #34 into master will not change coverage

@@             master        #34   diff @@
==========================================
  Files             2          2          
  Lines           269        269          
  Methods          57         57          
  Messages          0          0          
  Branches         38         38          
==========================================
  Hits            242        242          
  Misses           27         27          
  Partials          0          0

Powered by Codecov. Last update d26a20a...390eb34

timonreinhard commented 8 years ago

There are two ways of detecting when a device has left the network:

  1. Subscribe to device updates and listen for error events. (recommended)
  2. Check for the error callback when calling a device action.

The discover method is meant to find previously unknown devices. Your change would however have the effect of discovering already known devices possibly over and over again which is not intended (thus the comment in the original code). As your change reverts this design decision that was well-considered I unfortunately have to reject it. I would however be happy to accept a pull-request that introduces a list method that works in the way you described above if you feel there is a need for such a feature.

According to your feedback I changed the discover method (c6d9dc12ecb3199702a5bec37ae28b4de77f8946) to return devices that have previously been in an offline/error state and went back online. Before releasing 1.0 there will likely be a refactoring of the discovery and error handling that addresses connection related issues more consequently.