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 Argenteam Provider #705

Closed
wants to merge 2 commits into from
Closed

Conversation

juanmhidalgo
Copy link

Added Argenteam Provider (#595).

@juanmhidalgo juanmhidalgo changed the title Argenteam Provider Added Argenteam Provider Oct 15, 2016
@coveralls
Copy link

coveralls commented Oct 15, 2016

Coverage Status

Coverage increased (+0.08%) to 91.904% when pulling b8dde09 on juanmhidalgo:argenteam into dd74383 on Diaoul:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 91.904% when pulling b8dde09 on juanmhidalgo:argenteam into dd74383 on Diaoul:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 91.904% when pulling b8dde09 on juanmhidalgo:argenteam into dd74383 on Diaoul:master.

Copy link
Owner

@Diaoul Diaoul left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I requested some changes (see review comments).
Please use 120 characters line wrapping and ensure the docstrings are consistent. Make sure logging is accurate and takes place in the right spot.

@@ -75,7 +75,8 @@ def find_version(*file_paths):
'shooter = subliminal.providers.shooter:ShooterProvider',
'subscenter = subliminal.providers.subscenter:SubsCenterProvider',
'thesubdb = subliminal.providers.thesubdb:TheSubDBProvider',
'tvsubtitles = subliminal.providers.tvsubtitles:TVsubtitlesProvider'
'tvsubtitles = subliminal.providers.tvsubtitles:TVsubtitlesProvider',
'argenteam = subliminal.providers.argenteam:ArgenteamProvider'
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep it alphabetically ordered

self.episode = episode
self.release = release
self.version = version
logger.info('Release %r Version %r', release, version)
Copy link
Owner

Choose a reason for hiding this comment

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

Remove this log.

return self.download_link

def get_matches(self, video):
logger.info('GetMatches')
Copy link
Owner

Choose a reason for hiding this comment

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

Remove this log.

def terminate(self):
self.session.close()

def search_show_id(self, series, year=None):
Copy link
Owner

Choose a reason for hiding this comment

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

If the year parameter isn't used, remove it and update the docs.

:param year: year of the series, if any.
:type year: int or None
:return: the show id, if any.
:rtype: int or None
Copy link
Owner

Choose a reason for hiding this comment

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

The docstring doesn't match the function

Copy link
Owner

Choose a reason for hiding this comment

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

Docstring not accurate.


episode_id = self.search_episode_id(series, season, episode)
if episode_id is None:
logger.error('No show id found for %r', series)
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than here

assert len(extensions) == 9
assert extensions == ['addic7ed', 'de7cidda', 'legendastv', 'opensubtitles', 'podnapisi', 'shooter', 'subscenter',
assert len(extensions) == 10
assert extensions == ['addic7ed', 'argenteam', 'de7cidda', 'legendastv',
Copy link
Owner

Choose a reason for hiding this comment

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

Put argenteam after de7cidda

for s in r['subtitles']:
sub = ArgenteamSubtitle(
language, s['uri'], series,
season, episode, r['team'], r['tags'])
Copy link
Owner

Choose a reason for hiding this comment

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

Use 120 characters line wrapping

return []

r = self.session.get(self.API_URL + 'episode',
params={'id': episode_id}, timeout=10)
Copy link
Owner

Choose a reason for hiding this comment

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

Use 120 characters line wrapping

query = '%s S%#02dE%#02d' % (series, season, episode)
logger.info('Searching show id for %r', query)
r = self.session.get(self.API_URL + 'search',
params={'q': query}, timeout=10)
Copy link
Owner

Choose a reason for hiding this comment

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

Use 120 characters line wrapping

@juanmhidalgo
Copy link
Author

@Diaoul Thanks for your feedback.
I pushed some commits with fixes.

Copy link
Owner

@Diaoul Diaoul left a comment

Choose a reason for hiding this comment

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

Please fix the above and squash commits. If there is any further changes I will make them after merging.

r.raise_for_status()
results = json.loads(r.text)
episode_id = None
if results['total'] == 1:
episode_id = results['results'][0]['id']
else:
logger.error('No show id found for %r', series)
Copy link
Owner

Choose a reason for hiding this comment

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

I think you mean episode_id here.
Also the docstring of that function is not accurate.

@@ -69,13 +69,14 @@ def find_version(*file_paths):
entry_points={
'subliminal.providers': [
'addic7ed = subliminal.providers.addic7ed:Addic7edProvider',
'argenteam = subliminal.providers.argenteam:ArgenteamProvider'
Copy link
Owner

Choose a reason for hiding this comment

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

There is a coma missing at the end.

provider_name = 'argenteam'

def __init__(self, language, download_link,
series, season, episode, release, version, *args, **kwargs):
Copy link
Owner

Choose a reason for hiding this comment

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

Line wrap to 120.

# format
if video.format and self.version and video.format.lower() in self.version.lower():
matches.add('format')
matches |= guess_matches(
Copy link
Owner

Choose a reason for hiding this comment

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

Line wrap to 120.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@juanmhidalgo you missed this one. Put everything in one line


def initialize(self):
self.session = Session()
self.session.headers = {
Copy link
Owner

Choose a reason for hiding this comment

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

Line wrap to 120.

"""
# make the search
logger.info('Searching show id for %r', series)
r = self.session.get(self.API_URL + 'search',
Copy link
Owner

Choose a reason for hiding this comment

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

Line wrap to 120.

:param year: year of the series, if any.
:type year: int or None
:return: the show id, if any.
:rtype: int or None
Copy link
Owner

Choose a reason for hiding this comment

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

Docstring not accurate.

if results['total'] == 1:
episode_id = results['results'][0]['id']
else:
logger.error('No show id found for %r', series)
Copy link
Owner

Choose a reason for hiding this comment

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

You mean episode_id here I believe.

if episode_id is None:
return []

r = self.session.get(self.API_URL + 'episode',
Copy link
Owner

Choose a reason for hiding this comment

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

Line wrap to 120.

@protron
Copy link

protron commented Dec 3, 2016

@juanmhidalgo It's a shame this provider being so close to been merged, and now it's broken. I could try fixing it, but I guess I would have to create a new PR. Based on your branch of course. But maybe it still feels like stealing your work (your commits will still be there). Please let us know if you already give up on this.

@coveralls
Copy link

coveralls commented Jan 8, 2017

Coverage Status

Coverage increased (+0.08%) to 91.904% when pulling 9745790 on juanmhidalgo:argenteam into dd74383 on Diaoul:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 91.904% when pulling 9745790 on juanmhidalgo:argenteam into dd74383 on Diaoul:master.

@juanmhidalgo
Copy link
Author

Hi @protron I updated the PR. Let me know if it is ok or not.

@fernandog
Copy link
Collaborator

@juanmhidalgo you missed one request change: #705 (comment)
Put everything in one line

@ratoaq2
Copy link
Collaborator

ratoaq2 commented Feb 17, 2019

This PR is against master. It should be against develop branch

@ratoaq2
Copy link
Collaborator

ratoaq2 commented Feb 23, 2019

Fixed by 41f5c15
Thanks

@ratoaq2 ratoaq2 closed this Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants