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

0.7.x #404

Merged
merged 28 commits into from Mar 4, 2015
Merged

0.7.x #404

merged 28 commits into from Mar 4, 2015

Conversation

@caronc
Copy link

caronc commented Sep 18, 2014

I've been maintaining a plugin (forum) to your fantastic subtitle fetcher you've written. The plugin is for NZBGet (a UseNet solution). You can see this blog if interested. I'm only utilizing the 0.7.x branch, but have added some stability to it along with back porting older code so that it works with people running older versions of python (v2.6+) (like myself). The backporting specifically addresses an old ticket closed. I adapted to the podnapsi's new page layout and added a few other minor features. I just thought I should share my changes back!

Just added support for quotes which should fix issue 378 and issue 412 for you.

caronc added 9 commits Sep 18, 2014
…t providers being downloaded. Just download 1 (the best matched); bugfix
…ing Impaired (HI) first before non-HI or vs versa. This is kind of an extension to the bestscore enhancment already added in a previous commit.
…bliminal. Provider now supports new page layout
@caronc

This comment has been minimized.

Copy link
Author

caronc commented Oct 18, 2014

Quote support isn't pushed into the master branch yet, but i'm sure if someone beats me to it; it's pretty straight forward what i did (feel free to cherry pick away). I incremented the DogPile caching version too just to ensure no one references a local copy of their old quoted cached content.

@Diaoul

This comment has been minimized.

Copy link

Diaoul commented on 1e9588e Oct 19, 2014

Why is this needed? Do you have an example?

This comment has been minimized.

Copy link

Diaoul replied Oct 19, 2014

NVM just saw you referenced the issues in the PR. However I think the issue is more with guessit replacing dots, I believe I reported this already a long time ago.

This comment has been minimized.

Copy link

Diaoul replied Oct 19, 2014

This comment has been minimized.

Copy link
Owner Author

caronc replied Oct 19, 2014

That probably was a valid fix too that you referenced; but the quote issue only affects 'some' hits. I mean you can easily use subliminal and fetch Marvel Agent's of S.H.I.E.L.D. from opensubtitles. But if you watch carefully while debug mode is enabled (enable all providers); you can see that it skips over Addic7ed even though subs are available there. The difference is that opensubtitles stores that series without the quote in it's title. Where as Addic7ed stores the show with the quote.

ExampleURL: http://www.addic7ed.com/serie/Marvel%27s_Agents_of_S.H.I.E.L.D./1/1/Pilot (%27 being a quote).

The bug originally just spawned from a kind tester of a wrapper I have for your awesome tool. Here was the tester's initial report.

I can assure you; this patch does work (well for me anyway)! :)

This comment has been minimized.

Copy link

kir4h replied Apr 11, 2015

Hi,

Any chance of merging into the 0.8.0 branch? I'm using 0.8.0-dev because of some dependencies incompatibilities with other modules on 0.7.x

This comment has been minimized.

Copy link
Owner Author

caronc replied Apr 14, 2015

Sorry Kabracity, I'll probably eventually get around to upgrading to 0.8.0, but v0.7.x is so stable for me at this time. Is it just this patch specifically you'd like ported? What's the problem you're having?

This comment has been minimized.

Copy link

kir4h replied Apr 15, 2015

No worries @caronc ; I upgraded to 0.8.0 because of this compatibility issues with shared libraries (I upgraded flexget and the conflict started) and also because I thought it would be the near future. Not sure if @Diaoul is still spending time on it or development is paused until further notice.
I can try to replicate your patch in 0.8.0 and then do a commit; what I don't want is to just patch it locally.

The problem I am having is the same you mentioned in fact: Greys Anatomy + Addic7ed provided failing to find the subtitles because of the quote. I'd like to stick with Addic7ed as I find it's the most reliable provider.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 11, 2014

Coverage Status

Changes Unknown when pulling 3c7634f on caronc:0.7.x into * on Diaoul:0.7.x*.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 30, 2014

Coverage Status

Changes Unknown when pulling 7fef3cf on caronc:0.7.x into * on Diaoul:0.7.x*.

the flexibility for 3rd party apps that wrap the subliminal framework to
do their own guess management. The default behaviour of this fuction
will remain the same as it always has. This new feature will only kick
in if the video is specified to be used instead.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 7, 2014

Coverage Status

Changes Unknown when pulling e6bd704 on caronc:0.7.x into * on Diaoul:0.7.x*.

@Diaoul Diaoul mentioned this pull request Dec 23, 2014
caronc added 6 commits Jan 26, 2015
Hopefully the Addic7ed administrator can respond to my email inquiring as to why they are blocking us by the User-Agent string.

Automation is the 21st century of the internet; No one wants to click 8
times past banners just to get a 1KB (in size) subtitle. Most people
have Ad blocking software and don't even see these banners anyway. There
are many other ways to get people on board with helping them out
financially (if that's what this is about), and at the same time
accomodate those who've automated their service. I will roll back this
commit when we can come to a better resolution.
Hopefully the Addic7ed administrator can respond to my email inquiring as to why they are blocking us by the User-Agent string.

Automation is the 21st century of the internet; No one wants to click 8
times past banners just to get a 1KB (in size) subtitle. Most people
have Ad blocking software and don't even see these banners anyway. There
are many other ways to get people on board with helping them out
financially (if that's what this is about), and at the same time
accomodate those who've automated their service. I will roll back this
commit when we can come to a better resolution.
addic7ed_group = parser.add_argument_group('addic7ed')
addic7ed_group.add_argument('--addic7ed-username', metavar='USERNAME', help='username for addic7ed provider')
addic7ed_group.add_argument('--addic7ed-password', metavar='PASSWORD', help='password for addic7ed provider')

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

Why remove this?

This comment has been minimized.

Copy link
@caronc

caronc Feb 1, 2015

Author

see my comment on your next review about this provider (with respect to #428)

self.session.headers = {
'User-Agent': RANDOM_USER_AGENT,
'Referer': self.server,
}

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

Why use a random user agent here? I'm generally against this because servers should be allowed to block subliminal activity. Although I don't want them too, it should be possible. We should contact the provider to know the reason why they would want to block us and have an agreement.

This comment has been minimized.

Copy link
@caronc

caronc Feb 1, 2015

Author

I totally agree with you but Addic7ted has recently blocked all access their site 'unless' we do this. I even forked my nzbget-subliminal code into using the name nokarma and explain here why. It is part of my later commits to this branch.

In short, I've attempted to make contact with them and after a week (now it's been about 3) without hearing back from them; i just did this dirty move so we could still use the provider. I can roll this back if you want; it will break this provider though (see #428).

@@ -108,7 +82,7 @@ def get(self, url, params=None):
"""
try:
r = self.session.get(self.server + url, params=params, timeout=10)
r = self.session.get(self.server + url, params=params, timeout=30)

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

I think 10s timeout is plenty, if the provider doesn't respond by then, they are experiencing serious trouble.

This comment has been minimized.

Copy link
@caronc

caronc Feb 1, 2015

Author

I think i had a failure one day and got lazy and changed the value; i'll flip it back (in both cases you pointed out)

show_ids[html_show.string.lower()] = int(html_show['href'][6:])
show_ids[
IGNORED_CHARACTERS_RE.sub('', html_show.string).lower()] = \
int(html_show['href'][6:])

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

You should rename IGNORED_CHARACTERS_RE to something more cache-specific and create a sanitize_cache function (or similar) to put this logic.

@@ -150,10 +126,11 @@ def find_show_id(self, series):

def query(self, series, season):
show_ids = self.get_show_ids()
if series.lower() in show_ids:
show_id = show_ids[series.lower()]
_series = IGNORED_CHARACTERS_RE.sub('', series).lower()

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

This name is confusing, please make this explicit such as sanitized_series

This comment has been minimized.

Copy link
@caronc

caronc Feb 1, 2015

Author

Makes sense; i totally agree with the naming convention your suggestion. Will change (all references you pointed out)

@@ -180,7 +157,7 @@ def list_subtitles(self, video, languages):

def download_subtitle(self, subtitle):
try:
r = self.session.get(self.server + subtitle.download_link, timeout=10,
r = self.session.get(self.server + subtitle.download_link, timeout=30,

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

Same comment about the timeout here.

if video.series and self.series.lower() == video.series.lower():
if video.series and \
IGNORED_CHARACTERS_RE.sub('', self.series).lower() == \
IGNORED_CHARACTERS_RE.sub('', video.series).lower():

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

Maybe worth adding a sanitized_series property on the video object what do you think?
Edit: hmm changed my mind. There is no point in calling the same functions at two different places (here and in the object's property)

except AttributeError:
# there simply wasn't enough information in the TV Show
# gracefully handle this instead of crashing :)
break

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

Have you added a unittest for this? I think this is nice to handle it but maybe we need to be more specific about it by outputing a log message.

This comment has been minimized.

Copy link
@caronc

caronc Feb 1, 2015

Author

Let me see what i can do about this. There should be no reason i can't accommodate both of your very valid points.

@@ -153,7 +199,7 @@ def download_subtitle(self, subtitle):
raise ProviderNotAvailable('Timeout after 10 seconds')
if r.status_code != 200:
raise ProviderNotAvailable('Request failed with status code %d' % r.status_code)
with zipfile.ZipFile(io.BytesIO(r.content)) as zf:
with contextlib.closing(zipfile.ZipFile(io.BytesIO(r.content))) as zf:

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

This is why I hate python 2.6...

.replace('.', ' ').strip().lower()
else:
_series = _series.group(1)

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

A little comment on this and an explicit name instead of _series would be nice 🌟

continue
show = show.group(1)

if show == _series:

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

Same here

@@ -30,131 +30,61 @@ def test_find_show_id(self):
def test_find_show_id_error(self):
with self.Provider() as provider:
show_id = provider.find_show_id('the big how i met your mother')
self.assertIsNone(show_id)
self.assertEqual(show_id, None)

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

2.6 compat strikes again...

self.title,
self.year,
))
return hash(self.title)

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

Is this used anywhere?

This comment has been minimized.

Copy link
@caronc

caronc Feb 1, 2015

Author

At one point i was using it for the NZBGet plugin I wrote. It allows you to filter() content and eliminate duplicate videos found. the == operator or in operator could be used when comparing a big list of videos.

I can remove it if you want; i realize your not using it internally for anything It just makes it easier for people (like me) who use your tool's lower level functions to handle batches etc.

This comment has been minimized.

Copy link
@Diaoul

Diaoul Feb 1, 2015

Owner

I'm OK to keep it as it makes total sense for library users, I didn't see direct use of this in the library so I just wondered why add this :)

def __eq__(self, other):
return self.__class__.__name__ == other.__class__.__name__\
and self.title == other.title\
and self.year == other.year

This comment has been minimized.

Copy link
@Diaoul

Diaoul Jan 31, 2015

Owner

Or this?

@Diaoul

This comment has been minimized.

Copy link

Diaoul commented on 3acbefb Jan 31, 2015

OK now I understand. I've been contacted by Addic7ed developers in April 2014 and they wanted to know what information we would need from them because they were interested in building an API.

They didn't respond to my last email so I don't think they developed that API.

This comment has been minimized.

Copy link

sioban replied Feb 1, 2015

At least leaving the choice to the user would be a good idea.
I mean, using an alternate user agent might be an option.

This comment has been minimized.

Copy link

pannal replied Sep 11, 2015

Is this problem still existing?

This comment has been minimized.

Copy link

Diaoul replied Sep 11, 2015

I don't think so.

This comment has been minimized.

Copy link

pannal replied Sep 12, 2015

Well, still getting ERROR (logger:31) - subliminal.api: Unexpected error in provider 'addic7ed', discarding it errors. Are you sure? How can I get further information on that error? Not seeing anything further in "com.plexapp.agents.subliminal.log", "com.plexapp.system.log", "Plex Media Server.log".

This comment has been minimized.

Copy link
Owner Author

caronc replied Sep 12, 2015

Just chiming in with a bit of confusion. Are you receiving your errors using this fork which has this patch in place? Or are you getting the errors by other means? I realize this might be a bit of a rhetorical question because I can't imagine any Plex addons are using anything but the upstream branch.

This comment has been minimized.

Copy link

pannal replied Sep 12, 2015

I guess i was wrong about this one, as the errors I've mentioned are unrelated and self-generated. I'm currently playing around in the Plex sandbox.

@Diaoul

This comment has been minimized.

Copy link
Owner

Diaoul commented Jan 31, 2015

I just reviewed your changes. There are just a few points:

  • Python 2.6 is from eons ago, it makes the code less readable and not exactly python3 friendly, would you agree to drop this? What system are you running subliminal on to be that outdated?
  • I'd like to merge and publish this as a new (and hopefully last) release of the 0.7.x branch and rebase 0.8 on this.
@caronc

This comment has been minimized.

Copy link
Author

caronc commented Feb 1, 2015

I realize Python v2.6 is old, but I'm old fashioned too. I'd trade stability over bleeding edge any day. I run Red Hat 6.6 on all my production systems at work and CentOS 6.6 at home. As a result, I'm stuck with it's shipped version of Python 2.6. I've even packaged all of your software in RPMs which makes it so easy to deploy your fantastic tool. :)

I intend on updating to CentOS 7 soon which will bring me to Python v2.7. I'll still be old; but we'll at least be on the same page. :)

I'll read your review notes, I'll reply to them individually and do my best to explain them; from there you can let me know how you want me to proceed.

@Diaoul

This comment has been minimized.

Copy link
Owner

Diaoul commented Feb 1, 2015

I don't mind the dict and set comprehension stuff for 2.6 compat, what bothers me is more unittests that are less readable (and outputs less readable failures) and the zipfile stuff.

Maybe for 2.6 compat we can use 2.7+ syntax with unittest2?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 1, 2015

Coverage Status

Changes Unknown when pulling 407cbe1 on caronc:0.7.x into * on Diaoul:0.7.x*.

@caronc

This comment has been minimized.

Copy link
Author

caronc commented Feb 1, 2015

@sioban: just to avoid confusion, an option as you suggest would prove to be unnecessary (IMO). At this time, the proper User-Agent (Subliminal v0.7) is correctly applied to all providers except Addic7ed (as of very recently). If I were to revert back and apply the same User-Agent for Addic7ed too (as an option), then the provider simply won't work.
@Diaoul: I've accommodated most of the core requests you made, there are still some lingering I'll try to get to. Travis CI is really hit and miss eh? Today i passed, but yesterday i fail because providers are unavailable from the testing host. Oh well; right now things are working great, so that's a good thing! :)

caronc added a commit to caronc/nzb-subliminal that referenced this pull request Feb 1, 2015
@Diaoul

This comment has been minimized.

Copy link
Owner

Diaoul commented Feb 2, 2015

That's the problem with the unittests. I think the best for unittest would be to have a mock proxy that forwards the request to the original website, caches the response and send it back to the client as-is. If the original website fails to respond, the cache is used. So instead of testing against real servers we test against real or last time servers. That should be enough.

I can host this mock proxy locally or on free Heroku dyno I think.

I'll merge this and eventually make a few changes so I can make a new bugfix release of subliminal this week.

@caronc

This comment has been minimized.

Copy link
Author

caronc commented Feb 2, 2015

Sure! Thanks for accepting the pull request btw. I'm looking forward to dropping all my patch files! :)

@j-manu

This comment has been minimized.

Copy link

j-manu commented Feb 23, 2015

@Diaoul What you describe - a mock proxy - is handled in ruby by a gem called VCR. There are two python ports for it - https://github.com/sigmavirus24/betamax & https://github.com/kevin1024/vcrpy

Diaoul added a commit that referenced this pull request Mar 4, 2015
0.7.x
@Diaoul Diaoul merged commit 8c040f2 into Diaoul:0.7.x Mar 4, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@Diaoul

This comment has been minimized.

Copy link

Diaoul commented on 1e9588e Oct 19, 2014

Why is this needed? Do you have an example?

This comment has been minimized.

Copy link

Diaoul replied Oct 19, 2014

NVM just saw you referenced the issues in the PR. However I think the issue is more with guessit replacing dots, I believe I reported this already a long time ago.

This comment has been minimized.

Copy link

Diaoul replied Oct 19, 2014

This comment has been minimized.

Copy link
Owner Author

caronc replied Oct 19, 2014

That probably was a valid fix too that you referenced; but the quote issue only affects 'some' hits. I mean you can easily use subliminal and fetch Marvel Agent's of S.H.I.E.L.D. from opensubtitles. But if you watch carefully while debug mode is enabled (enable all providers); you can see that it skips over Addic7ed even though subs are available there. The difference is that opensubtitles stores that series without the quote in it's title. Where as Addic7ed stores the show with the quote.

ExampleURL: http://www.addic7ed.com/serie/Marvel%27s_Agents_of_S.H.I.E.L.D./1/1/Pilot (%27 being a quote).

The bug originally just spawned from a kind tester of a wrapper I have for your awesome tool. Here was the tester's initial report.

I can assure you; this patch does work (well for me anyway)! :)

This comment has been minimized.

Copy link

kir4h replied Apr 11, 2015

Hi,

Any chance of merging into the 0.8.0 branch? I'm using 0.8.0-dev because of some dependencies incompatibilities with other modules on 0.7.x

This comment has been minimized.

Copy link
Owner Author

caronc replied Apr 14, 2015

Sorry Kabracity, I'll probably eventually get around to upgrading to 0.8.0, but v0.7.x is so stable for me at this time. Is it just this patch specifically you'd like ported? What's the problem you're having?

This comment has been minimized.

Copy link

kir4h replied Apr 15, 2015

No worries @caronc ; I upgraded to 0.8.0 because of this compatibility issues with shared libraries (I upgraded flexget and the conflict started) and also because I thought it would be the near future. Not sure if @Diaoul is still spending time on it or development is paused until further notice.
I can try to replicate your patch in 0.8.0 and then do a commit; what I don't want is to just patch it locally.

The problem I am having is the same you mentioned in fact: Greys Anatomy + Addic7ed provided failing to find the subtitles because of the quote. I'd like to stick with Addic7ed as I find it's the most reliable provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.