search

perbrage / sectoralarm

Sector Alarm Node.js Library
MIT License
23 stars 8 forks source link

Support for temperature sensors #3

Closed frli4797 closed 5 years ago

frli4797 commented 5 years ago

Adding support for temperature sensors from alarm.

Strangely enough all (if present) temperature sensors are enumerated in the GetOverview call, but no temperatures are present in the response from that call. A call to GetTemperatures has to be made separately to receive the temperatures. A possible improvement could be to make a call to status() to ensure that there actually are temperature sensors present. However I do not have access to an alarm that does not have sensors which means that I really could not explore options for being a bit more defensive in that sense.

Note: the word "temperature" is misspelled in the response from Sectoralarm.

perbrage commented 5 years ago

Nice work and nice addition. I also see that you did quite some style formatting and also fixed some of the tests when it comes to the JSON parsing. Thank you.

The misspelled stuff in their API is just horrible. And I have tried to correct it when I see it in this library's output. Did you do that?

perbrage commented 5 years ago

I don't have any sensors, so for me I get a returned empty array. So that seems fine.

I looked at the test you added, and I really think that you should filter away the useless parameters and try to correct the spelling.

Or I can do that after merging the pull request.

frli4797 commented 5 years ago

Nice work and nice addition. I also see that you did quite some style formatting and also fixed some of the tests when it comes to the JSON parsing. Thank you.

The misspelled stuff in their API is just horrible. And I have tried to correct it when I see it in this library's output. Did you do that?

Yes, changed the spelling of "Tempratures" to "Temperatures". :)

frli4797 commented 5 years ago

I don't have any sensors, so for me I get a returned empty array. So that seems fine.

I looked at the test you added, and I really think that you should filter away the useless parameters and try to correct the spelling.

Or I can do that after merging the pull request.

No, worries. I thought about fixing that, but it slipped my mind. Amended the PR with this change. Thanks for the positive feedback.

BTW. I'm also working with a Homebridge/Homekit plugin for Sector in case you're a Apple guy. So this is the reason to why I'm particularly interested in getting some more functionality into you excellent little project. (However the plugin is very much boilerplate still.)

frli4797 commented 5 years ago

Sorry, something went sour when I committed the "unnecessary" fields fix. Looks kind of bad with two identical commits, but...

perbrage commented 5 years ago

I will have a look tomorrow! Cheers

perbrage commented 5 years ago

Nice work! Merged it!

perbrage commented 5 years ago

Btw, you don't happen to have a Yale doorman lock or some such? In my node-red version of the project I have a request to add support for that in this general library. But I don't have such a lock and are looking for someone to help me develop it. I got some information on how to communicate with it, but no way to test it.

frli4797 commented 5 years ago

Sorry. Wish I had ont though.

sön 28 okt. 2018 kl. 07:38 skrev Per Brage notifications@github.com:

Btw, you don't happen to have a Yale doorman lock or some such? In my node-red version of the project I have a request to add support for that in this general library. But I don't have such a lock and are looking for someone to help me develop it. I got some information on how to communicate with it, but no way to test it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/perbrage/sectoralarm/pull/3#issuecomment-433680581, or mute the thread https://github.com/notifications/unsubscribe-auth/AFg1qFRtACR6_ib7S4x1X6KXyJjfhlh9ks5upVDdgaJpZM4X84rV .

perbrage commented 5 years ago

No worries! And thank you for the contribution you made!