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

Subtitles #406

Merged
merged 4 commits into from
Sep 4, 2023
Merged

Subtitles #406

merged 4 commits into from
Sep 4, 2023

Conversation

pacoccino
Copy link
Contributor

@pacoccino pacoccino commented Sep 2, 2023

I propose this code changes to enable subtitles downloading.
It is the first time for me using python&django so it’s a bit of discovery ;)

I don’t know if I’ve done everything that would have been necessary to add this feature, but it does what I need as it is.

I worked on this as I really need to have subtitles downloaded and I don’t need/want something as heavy as tubearchivist.

#7 #367

@meeb
Copy link
Owner

meeb commented Sep 4, 2023

Thanks for the contribution! This looks fine to me. The only thing I would suggest is some model form validation as a Django validator that confirms that the subtitle language field is actually a comma-separated array of two letter, lower case values. Currently as far as I can see this will otherwise let people pass arbitrary data to yt-dlp's API which is probably worth putting some guide rails on.

Something like (untested, just typed ad-hoc):

def validate_is_subtitle_lang_list(value):
    parts = value.split(',')
    if not parts:
        return True
    subtitle_lang_chars = 'abcdefghijklmnopqrstuvwxyz'
    for i, lang in enumerate(parts):
        if len(lang) != 2:
            raise ValidationError(f'Subtitle lang code at position {i} is {len(lang)} chars, must be 2 chars')
        for c in lang:
            if c not in subtitle_lang_chars:
                raise ValidationError(f'Subtitle lang code at position {i} must be a-z lowercase only, got {lang}')
    return True

As you're new to Django, here's how validators are integrated to save you looking it up: https://docs.djangoproject.com/en/4.2/ref/validators/ - you can add the validator to your new sub_langs field. Thanks again for the PR, most appreciated.

@pacoccino
Copy link
Contributor Author

Nice catch, I added a validator, but opted for a regex instead, matches better the options available

Another thing is that it won't download subtitle for already downloaded videos, I doubt you can use yt-dlp just for downloading subtitle without video but I'll check that

@meeb meeb merged commit 777cdb5 into meeb:main Sep 4, 2023
@meeb
Copy link
Owner

meeb commented Sep 4, 2023

Nice, thanks! I'll give this a test and merge it into the next release.

@tehniemer
Copy link

Will this grab subtitles for already downloaded media?

@meeb
Copy link
Owner

meeb commented Sep 7, 2023

Alas, no it won't.

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.

3 participants