feat: allow specifying the same option multiple times using an array #26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
YoshiWalsh
added a commit
to YoshiWalsh/subtitle-indexer
that referenced
this pull request
Nov 3, 2020
This requires changes which have yet to be merged in an upstream project: phaux/node-ffmpeg-stream#26 You will need to manually build and install JoshuaWalsh/node-ffmpeg-stream#feature/issue-25 to use this branch.
|
Hi @phaux , if you have a chance could you please review this? I'd love to see this functionality included in the npm version. |
|
Sorry for not responding for so long. It looks like the CI doesn't pass but it doesn't show the status here for some reason. I'm gonna merge this anyways and then publish a new version. |
phaux
changed the title
|
If you have any suggestions of how I can add test coverage for this I would be happy to do so. |
|
It's fine. I will publish a new version after the CI turns green. |
|
Published! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #25.
I haven't added any test coverage for this, sorry. Based on the current test suite I think it would be hard to do. If you'd like me to add tests I think I'll have to bring in
ffprobeandffprobe-staticas devDependencies.I have tested this in my own application using
npm linkthough, and it's working for my requirements.A more DRY way of implementing this feature would be to make
valuealways an array using a ternary expression, but I think the way I've implemented it makes the intent more obvious when reading the code. If you want me to change it to be more DRY I'm happy to do so.