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

Added a spanish subtitle provider subtitulamos.tv #7518

Merged
merged 7 commits into from
Jan 5, 2020

Conversation

porzino
Copy link
Contributor

@porzino porzino commented Dec 21, 2019

Added a spanish subtitle provider subtitulamos.tv

@medariox
Copy link
Contributor

Good job on this PR 👍

Can you please fix the flake issues D204 1 blank line required after class docstring here? https://travis-ci.org/pymedusa/Medusa/jobs/628149935#L1932-L1944
About the D100 issues you can exclude them by adding the file here: https://github.com/pymedusa/Medusa/blob/develop/setup.cfg#L171-L174

Added medusa/subtitle_providers/subtitulamos.py.
Fixed the flake issues D204.
Fixed the flake issues W293.
@porzino
Copy link
Contributor Author

porzino commented Dec 23, 2019

Is it OK now?

@medariox
Copy link
Contributor

Yes, thank you! Will try to get this reviewed and merged in the next few days.

@porzino
Copy link
Contributor Author

porzino commented Dec 24, 2019

Thanks a lot!!!

Copy link
Contributor

@medariox medariox left a comment

Choose a reason for hiding this comment

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

Just a few minor changes and it can be merged.


"""
# make the search
logger.info('%s: Searching episode url for %s, season %d, episode %d', self.__class__.__name__.upper(), series, season, episode)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is necessary to prepend every provider log with the name. Looks like no other provider does it either. Please remove it from all the logs.

r.raise_for_status()

if r.status_code != 200:
logger.error('%s: Error getting episode url', self.__class__.__name__.upper())
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this warning instead.

# get the episode url
episode_url = self._search_url_titles(series, season, episode, year)
if episode_url is None:
logger.warning('%s: No episode url found for %s, season %d, episode %d', self.__class__.__name__.upper(), series, season, episode)
Copy link
Contributor

@medariox medariox Dec 26, 2019

Choose a reason for hiding this comment

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

I think making this warning is going to be too much. This might occur quite often, better to make it info.

soup = ParserBeautifulSoup(r.content, ['lxml', 'html.parser'])

# get episode title
logger.info('%s: Getting episode title', self.__class__.__name__.upper())
Copy link
Contributor

Choose a reason for hiding this comment

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

Better make this debug.

title_pattern = re.compile('{}x{:02d} - (.+)'.format(season, episode))
title = title_pattern.search(soup.select('.episode-name')[0].get_text().strip().lower()).group(1)

logger.info('%s: Episode title found: "%s"', self.__class__.__name__.upper(), title.upper())
Copy link
Contributor

Choose a reason for hiding this comment

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

Better make this debug.

logger.info('%s: Getting episode title', self.__class__.__name__.upper())

title_pattern = re.compile('{}x{:02d} - (.+)'.format(season, episode))
title = title_pattern.search(soup.select('.episode-name')[0].get_text().strip().lower()).group(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use get_text(strip=True) instead of get_text().strip(). Also for the other occurrences.

# ignore incomplete subtitles
status = sub.find_next('div', class_='subtitle_buttons').contents[1]
if status.name != 'a':
logger.info('%s: Ignoring subtitle in [%s] not finished', self.__class__.__name__.upper(), language)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not very clear why the status.name should be a. Please explain what is going on here and improve the logger message.

@medariox medariox added this to the 0.3.10 milestone Dec 26, 2019
Minor changes made.
@porzino
Copy link
Contributor Author

porzino commented Jan 3, 2020

Sorry for the delay, but I was on Christmas holidays.
I have made the changes advised. I hope everything is right now.
Thanks for reviewed it.

@porzino porzino requested a review from medariox January 3, 2020 13:25
Problem with tabs resolved.
@medariox
Copy link
Contributor

medariox commented Jan 4, 2020

No worries, same here. Can you remove the master branch protection? So I can make a few final changes, see: https://help.github.com/en/github/administering-a-repository/configuring-protected-branches

@porzino
Copy link
Contributor Author

porzino commented Jan 4, 2020

I think I have remove the protection. Is it ok?

@medariox
Copy link
Contributor

medariox commented Jan 4, 2020

Nope, sadly I can't even push a new branch to your repository. 😢

Copy link
Contributor

@medariox medariox left a comment

Choose a reason for hiding this comment

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

Will open a new PR, thanks for the great work!

@medariox medariox merged commit e7c72c6 into pymedusa:develop Jan 5, 2020
@porzino
Copy link
Contributor Author

porzino commented Jan 15, 2020

Hi, sorry about the protection issue.
Thanks for your patience and work with this PR.

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