Skip to content
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

Handling unquoted codecs #6

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

Conversation

avimak
Copy link

@avimak avimak commented Jun 25, 2019

sometimes mdls returns unquoted codec names, i.e. ("H.264",AAC), so instead of failing the parsing process completely, we better try looking for Alphabet characters in it and return them as value in case we managed to find at least one non-digit/valid Alphabet char.

@brettz9
Copy link
Collaborator

brettz9 commented Jun 26, 2019

This looks good to me, but I hope we could get unit tests added for any new additions.

Though we might not be able to easily test mdls consistently, I think we could at least export the deserialize function (if not also to_js_type, though first converting to CamelCase) and checking that.

Would you mind adding a unit test--maybe Mocha--for your addition? With some testing framework in place, we can then sanity check with any new modifications (and also try to eventually add tests to cover other supported parsing scenarios).

@AWinterman
Copy link
Owner

I'm going to second the call for tests please.e

@AWinterman
Copy link
Owner

@avimak just checking in, do you intend to add tests?

@avimak
Copy link
Author

avimak commented Dec 2, 2019

sorry, can't find the time

@pdenya
Copy link

pdenya commented Oct 18, 2020

The "Now handling parenthesis in file names" commit only selectively escapes file names. Instead of escaping the file name it should just be wrapped in quotes, eg:

exec('mdls ' + (args ? args + ' ' : '') + `"${file}"`, function(err, raw_data) {

I have this adjusted on my fork at https://github.com/pdenya/node-mdls. I merged the rest of the changes from this PR and I'm also not planning to add tests so didn't think it was worth submitting another PR against.

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

Successfully merging this pull request may close these issues.

4 participants