-
Notifications
You must be signed in to change notification settings - Fork 28
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
Extend file type support #72
base: release/1.0.9
Are you sure you want to change the base?
Conversation
Added the ability to support all file types supported by `mkvmerge` (rebased to 1.0.9). I also renamed the `info_json` variable to `info` because to me it seems clear we are dealing with JSON.
if not verify_mkvmerge(mkvmerge_path=mkvmerge_path): | ||
raise FileNotFoundError('mkvmerge is not at the specified path, add it there or change' | ||
'the mkvmerge_path property') | ||
if file_path is not None: | ||
file_path = expanduser(file_path) | ||
info = json.loads(sp.check_output([self.mkvmerge_path, '-J', file_path]).decode()) | ||
if info['container']['recognized'] is True and info['container']['supported'] is True: |
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.
It seems that the info['container']['recognized']
check belongs to the verify_matroska
function, so we can reuse it in other checks down the road. And in that case, I think we would still need to use the verify_matroska
check here since we are "importing" a file.
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.
It is used in verify_recognized
, but I thought that check here is not enough, the file must also be supported, hence the and info['container']['supported'] is True
. I removed verify_matroska
because I found your package can work with file types other than MKV (I have tested it with MP4, but ideally it should work with the other supported file types, too) and then produce an MKV, and I wouldn't call the verify_supported
because we already have the JSON dictionary, we can query it directly.
I also find the code in Verifications.py
to be rather duplicated, and I think the info
dictionary could be useful to other users, so I have come up with this, a new function identify_file
(or maybe just identify
) which does the heavy lifting for verify_matroska
, verify_recognized
and verify_supported
:
def identify_file(file_path, mkvmerge_path='mkvmerge'):
"""Get information about about the source file. Same as `mvkmerge -J <file_path>`."""
if not isinstance(file_path, (str, os.PathLike)):
raise TypeError(f'"{file_path}" is not of type str or os.PathLike')
if not isfile(file_path):
raise FileNotFoundError(f'"{file_path}" does not exist')
try:
info = json.loads(sp.check_output([mkvmerge_path, '-J', file_path]).decode())
except sp.CalledProcessError:
raise ValueError(f'"{file_path}" could not be opened')
return info
def verify_matroska(file_path, mkvmerge_path='mkvmerge'):
"""Verify if a file is a Matroska file.
file_path (str):
Path of the file to be verified.
mkvmerge_path (str):
Alternate path to mkvmerge if it is not already in the $PATH variable.
"""
info = identify_file(file_path, mkvmerge_path)
return info['container']['type'] == 'Matroska'
def verify_recognized(file_path, mkvmerge_path='mkvmerge'):
"""Verify a file is recognized by mkvmerge.
file_path (str):
Path to the file to be verified.
mkvmerge_path (str):
Alternate path to mkvmerge if it is not already in the $PATH variable.
"""
info = identify_file(file_path, mkvmerge_path)
return info['container']['recognized']
def verify_supported(file_path, mkvmerge_path='mkvmerge'):
"""Verify a file is supported by mkvmerge.
file_path (str):
Path to the file to be verified.
mkvmerge_path (str):
Alternate path to mkvmerge if it is not already in the $PATH variable.
"""
info = identify_file(file_path, mkvmerge_path)
return info['container']['supported']
I have removed the verify_mkvmerge
check because I think it is not necessary here, the user can check it when they need, before using other functions which depend on mkvmerge
, instead of having the package check it every time. (I have also not updated the documentation.)
What do you think?
Added the ability to support all file types supported by
mkvmerge
(rebased to 1.0.9). I also renamed theinfo_json
variable toinfo
because to me it seems clear we are dealing with JSON.