-
-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Require Node.js 8, add known binaries, add multiple default argument sets #15
Conversation
@sindresorhus Bumped node version to 8, and excluded 6 from tests there. (As async doesn't work in 6, it's painful to implement this without async, and you probably would bump version yourself soon anyway.) |
index.js
Outdated
.catch(error => { | ||
const knownBinaryArgs = new Map([ | ||
['openssl', ['version']], | ||
['ffmpeg', ['-version']] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you include some more popular binaries with non---version
version flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I can't remember any, not a type of information I try hard to remember, and everything I try of the top of my head accepts --version
. But should probably put up a notice in readme incentivising people to file an issue if they find some.
I've added ffprobe
and ffplay
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(for the notice part, it would really be best if you formulate it and put where you want, otherwise we will end up with a couple of more rounds of back and forward about one sentence)
Fixes #12.