Add support for embed.ly #2447

Merged
merged 1 commit into from Mar 5, 2014

Conversation

Projects
None yet
2 participants
youtube_dl/extractor/generic.py
+ mobj = re.search(r'class=["\']embedly-embed["\'][^>]src=["\'][^"\']*url=(?P<url>[^&]+)', webpage)
+ if mobj is not None:
+ url = compat_urllib_parse.unquote(mobj.group('url'))
+ from . import _ALL_CLASSES
@phihag

phihag Feb 24, 2014

Collaborator

This looks like quite a hack. Why do we need it?

@rzhxeo

rzhxeo Feb 24, 2014

Contributor

Embed.ly also supports non-video sites.
I added this check to prevent failure in case of a webpage that uses embed.ly to embed some non-video content and also contains a video that is detected by code after this (e.g. jwplayer).
There is no proper way (?) to detect if an url is supported by youtube-dl, so I used this "hack".

@phihag

phihag Feb 26, 2014

Collaborator

But wouldn't it be the correct behavior to then download that video as well? Can you name an example URL that produces a detrimental behavior when these lines are removed?

@rzhxeo

rzhxeo Feb 26, 2014

Contributor

What video do your mean in your first question?
I don't have a example URL. I think you can remove the hack until we encounter a webpage that doesn't work.

@phihag

phihag Feb 26, 2014

Collaborator

I meant a webpage that uses embed.ly to embed some non-video content and also contains a video that is detected by code after this. I concur that the best action would be to wait for a test case, and then decide how we can exclude it or improve some other code.

Collaborator

phihag commented Feb 24, 2014

Can you add a test (or multiple, if they're using different domains or so) for this? Then we can check that the code still works after we modify parts of it.

Contributor

rzhxeo commented Feb 26, 2014

@phihag I've added a test.

Contributor

rzhxeo commented Feb 27, 2014

I've removed the hack and rebased.

phihag added a commit that referenced this pull request Mar 5, 2014

[generic] Add all test attributes for embedly (#2447)
In the future, we may want to not only print something, but throw an error for untested properties.
Collaborator

phihag commented Mar 5, 2014

Thanks, merged.

@phihag phihag merged commit 1b86cc4 into rg3:master Mar 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment