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

Optionally Ignore unknown TrackTypes #18

Merged
merged 14 commits into from
Jun 13, 2015
Merged

Optionally Ignore unknown TrackTypes #18

merged 14 commits into from
Jun 13, 2015

Conversation

nicholi
Copy link
Contributor

@nicholi nicholi commented Jun 12, 2015

Added an explicit typed exception to be caught in order to ignore unknown track types. Since you didn't have the Other (or Menu) section types implemented these will just throw exceptions making the library almost useless.

I also implemented the Other type as I wanted to use it. However Menu is still unimplemented, and any other strange sections MediaInfo might have in the future. My suggestion, make it ignore unknown track types by default rather than throw an exception. As you'll see I made the parameters to ignore optional and set to false, so it copies the current behavior.

… .mov). Also implement explicit Exception type for unknown track types. Optional parameters used to determine if exception is thrown, or unknown tracks are simply skipped (likely preferred).
@mhor
Copy link
Owner

mhor commented Jun 12, 2015

Thanks for PR, LGTM, before merging could you:

  • Fix code style issue (travis-build)
  • Add some unit test
  • Add documentation

@nicholi
Copy link
Contributor Author

nicholi commented Jun 12, 2015

Ok added documentation for the parameters, some unit tests, and any other minor style fixes.

mhor added a commit that referenced this pull request Jun 13, 2015
Optionally Ignore unknown TrackTypes
@mhor mhor merged commit ac3bc07 into mhor:master Jun 13, 2015
@mhor
Copy link
Owner

mhor commented Jun 13, 2015

Thanks @nicholi. Merge in master (relased in 1.3.0)

@mhor mhor mentioned this pull request Sep 16, 2015
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.

2 participants