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

Add Subtitulamos provider integration #1170

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

Nyaran
Copy link
Contributor

@Nyaran Nyaran commented Sep 22, 2024

Added Subtitulamos provider

This PR is partially based and replaces #826 (skipping the provider "tusubtitulo" as is not working anymore)

Also I added a way to run the cli from source code (subliminal/cli.py#L727-L728) , as is helpful to debug new additions (if you don't like it, tell me and I'll remove it)

Pending:

  • Add tests
  • Changelog entry
  • CI passes (except docs)

subliminal/cli.py Outdated Show resolved Hide resolved
@getzze
Copy link
Collaborator

getzze commented Oct 8, 2024

Thanks for updating this provider!

Don't worry too much about mypy (or if you need help let me know). It's better if coverage is high ;)

@Nyaran Nyaran marked this pull request as ready for review October 13, 2024 11:10
Comment on lines +233 to +238
# modify spanish latino subtitle language to only spanish and set hearing_impaired = True
# because if exists spanish and spanish latino subtitle for the same episode, the score will be
# higher with spanish subtitle. Spanish subtitle takes priority.
if language == Language('spa', 'MX'):
language = Language('spa')
hearing_impaired = True
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a hack that will stop working with changes to come to how hearing_impaired and foreign_only/forced subtitles are sorted. See #1175 for the first step.

Why do you need to transform a american-spanish subtitle to a european-spanish subtitle? It can be specified by having both languages (or only one) in the command line, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure, I thought the same when I saw that on the original proposal, but I decided to keep it in case there was something I missed. Let me perform some test and make the commit removing the block.

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