rachelnicole / robokitty

A DIY Cat (or dog. or human) Feeder powered by node
http://imcool.online/robokitty
GNU General Public License v2.0
773 stars 35 forks source link

Consider throwing or process.exit'ing if required vars are not supplied #20

Closed toddself closed 9 years ago

toddself commented 9 years ago

On https://github.com/rachelnicole/robokitty/blob/master/app.js#L10-L11

You definitely want someone to put that data in there. Since they're required (and the defaults are invalid) you might not want to do the shortcircuit || operator, but rather check to see if those variables are undefined and if they are print out usage instructions:

var token = process.env.PARTICLE_KEY
var deviceId = process.env.PHOTON_ID

if (!token || !deviceId) {
   console.log('You need to provide your PARTICLE_KEY and PHOTON_ID as environment variables before running this command')
   console.log('Ex: PARTICLE_KEY=test PHOTON_ID=test node app.js')
   process.exit()
}

(Or you could throw too, if app.js is ever require'd by another module, calling process.exit is a pretty rude thing to do to the including program :))

iabw commented 9 years ago

I still agree with this, but my comment was just me getting confused because #13 isn't fixed yet. :P

rachelnicole commented 9 years ago

Bout to read a long ass article about throwing errors and I'll fix this either tonight or first thing in the morning. :)

rachelnicole commented 9 years ago

@toddself @iabw I started on trying to work on this but my brain is mush. I scoped out the authentication stuff into it's own config file entirely, but I think the error handling needs to be worked in case of the particle id or token being invalid.

let me know what you think, if this is even the right direction or if I'm overly complicating things. Or if it's just wrong completely.

https://github.com/rachelnicole/robokitty/pull/24/files

toddself commented 9 years ago

I was going to recommend making a config file eventually so you don't have to pass in via env variables, but you've gone and done it!

process.exit() seems appropriate here too. LGTM!

rachelnicole commented 9 years ago

💁✨

MilanLoveless commented 9 years ago

Oops, I missed this discussion. I refactored to a "throw" because it's more testable. We could catch the error in app.js and then process.exit() though.