Skip to content
This repository has been archived by the owner on May 9, 2020. It is now read-only.

Check shntool and cuetools existance #23

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dichlofos
Copy link

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

When cuetools package is not installed, split2flac will fail with
cryptic error in sh script. So it's better to check that cuetools
programs exist in the $PATH.
@ftrvxmtrx
Copy link
Owner

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
Copy link
Author

  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
Copy link
Owner

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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants