RocHack / bb

Command line Blackboard client
MIT License
72 stars 8 forks source link

Warn users if curl is not installed, and exit #13

Closed jeremywrnr closed 9 years ago

jeremywrnr commented 9 years ago

Potentially take care of #4. Haven't tested with a machine that doesn't have curl however...

clehner commented 9 years ago

Looks good except for two thing:

  1. the which command writes to stdout on success, and since it's called in bb_request, that output gets added the request response, which may cause problems with parsing (although I'm not sure if it actually does cause problems). This could be addressed by using which curl >/dev/null. Also note you can use a command in an if statement, e.g.

    if which curl >/dev/null; then                                                                                                                                              
       curl_installed=1
       ...
  2. When I test without curl, with some bb commands I get the error twice:
$ bb courses
'curl' is not installed, install curl before running bb.
info: http://curl.haxx.se/docs/manpage.html
'curl' is not installed, install curl before running bb.
info: http://curl.haxx.se/docs/manpage.html

I think this is because of subshells, since there is indeed an exit called after the error is printed, but check_curl may be getting called the first time in a subshell so that the exit only returns to the caller of the subshell. This could probably be fixed by moving the call to check_curl, perhaps to the main level before any commands get executed.

jeremywrnr commented 9 years ago

Cool. I moved it the check to run right after all the constants are defined in the beginning of the file, so this should avoid any issues with parsing. Moving it there also negates any advantage remembering whether you already checked to see if they have curl since it will/(should?) only run once, so I took out the curl_installed variable.

I also googled it and it seems that using which is not a good way to do it (reasons to not use which), as some versions of which may not actually exit upon not finding the command it was looking for.

clehner commented 9 years ago

Hmm, looks like it may be better then to use curl --version or type/hash builtins.

jeremywrnr commented 9 years ago

Yep, in 520b643 I used the type builtin as suggested. Does that work for you w/o curl?

clehner commented 9 years ago

I get /usr/local/bin/bb: line 41: check_curl: command not found since the function is called before it's defined.

jeremywrnr commented 9 years ago

Ok, moved it to main to the main section in 0ddff412

clehner commented 9 years ago

Works for me! Merge at will.

jeremywrnr commented 9 years ago

Thank you!