ftrvxmtrx / split2flac

Split flac/ape/wv/wav + cue sheet into separate tracks
253 stars 34 forks source link

Check shntool and cuetools existance #23

Open dichlofos opened 10 years ago

dichlofos commented 10 years ago

It's better to check cuetools/shntool tools existance to avoid cryptic shell errors, like this:

/home/mvel/.local/bin/split2flac: line 431: [: 1: unary operator expected
Failed to get number of tracks from CUE sheet.
There may be an error in the sheet.
/home/mvel/.local/bin/split2flac: line 123: printf: `N': invalid format character
Running cueprint -n 1 -t /home/mvel/.local/bin/split2flac: line 443: cueprint: command not found
ftrvxmtrx commented 10 years ago

While I agree it might look cryptic, it's doesn't really matter, as the required dependencies are listed explicitly in both README file and the script itself. So it comes to why would anyone run the script without installing its dependencies. I don't think the checks are really required here. Can you elaborate?

dichlofos commented 10 years ago
  1. Nobody (including myself) cares about docs reading. I added these checks exactly after I run split2flac on my second machine where cuetools/shntools were NOT installed just as it was rather fresh Fedora installation. I just run my conversion function, I even forgot what tools/packages it uses internally. So I decided that extra sanity checking will not harm and saves my brain memory.
  2. These checks will not slow down entire splitting process.
  3. Explicitly listed requirements do not allow us to pass incorrect parameters to shell operators :)

Moreover, I'd add all deps checking, but these two are good starting point.

ftrvxmtrx commented 10 years ago

I'll merge it, just two small things. Can you please move these checks right before the arguments parsing, without adding more functions? And please use "command" instead of "which" (see its usage right after configuration saving). Thanks!