sandeepmistry / node-sensortag

Node.js lib for the TI SensorTag
MIT License
218 stars 111 forks source link

more output and error handling #32

Closed schnippe closed 9 years ago

schnippe commented 9 years ago

outputs an error stack instead of ideling. more user feedback about buttons to be pressed for less frustration

sandeepmistry commented 9 years ago

@schnippe thanks for submitting!

Want do you think about using a console.log or console.warn instead of adding error parameter to callback?

My reasoning behind it:

1) It's not a common case 2) Would introduce incompatibility with user's using existing version 3) Makes the API inconsistent, ideally all API calls would have a error parameter, but that's a bigger change.

schnippe commented 9 years ago

I am quite into error handling because it help maintaining a consistent state. I try to manage all cases. The introduction of incompatiblities is aceptable if it helps getting to a stable system.

I could not do any stuctured error handling in a main application if I had no access to the error state of the library.

I like using an error parameter in the callback very much because it is the standard node way of returning an error to a callback. Please see Node 0.12 Doc and search for Callback(

Incrementaly changing this everywhere where needed would help getting to API consistent with node.js and help transitioning to promises and generators.

sandeepmistry commented 9 years ago

It might be better to discuss this and #33 via email or gitter.

Moving forward, I'd like to:

These are "bigger" changes, so working on a branch/fork until complete would be great.

schnippe commented 9 years ago

@sandeepmistry, ok I agree I will use gitter I will try to merge -device and -sensortag if no one else is doing it at the moment. Meanwhile I am trying a distributed Noble approch using multiple raspberries and multiple NOBLE_HCI_DEVICE_IDs.

sandeepmistry commented 9 years ago

With recent changes, this PR is no longer applicable. It would have to be applied to noble-device now ...