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

Extend file type support #72

Open
wants to merge 1 commit into
base: release/1.0.9
Choose a base branch
from

Conversation

Rubo3
Copy link
Contributor

@Rubo3 Rubo3 commented Aug 12, 2021

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.

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.
@Rubo3 Rubo3 mentioned this pull request Aug 12, 2021
Comment on lines +97 to +103
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:
Copy link
Owner

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.

Copy link
Contributor Author

@Rubo3 Rubo3 Aug 14, 2021

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?

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