split2flac icon indicating copy to clipboard operation
split2flac copied to clipboard

Check shntool and cuetools existance

Open dichlofos opened this issue 11 years ago • 3 comments

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

dichlofos avatar Jul 22 '14 07:07 dichlofos

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?

ftrvxmtrx avatar Jul 27 '14 21:07 ftrvxmtrx

  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.

dichlofos avatar Jul 27 '14 21:07 dichlofos

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!

ftrvxmtrx avatar Jul 27 '14 21:07 ftrvxmtrx