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

Vimeo InfoExtractor #17

Closed
wants to merge 1 commit into from
Closed

Vimeo InfoExtractor #17

wants to merge 1 commit into from

Conversation

ppawel
Copy link

@ppawel ppawel commented Nov 6, 2010

Hey, this is my VimeoIE implementation that I pushed to bitbucket.org, below is your feedback which I still need to address. I will make changes in the code and let you know.

Below is what you wrote to me on bitbucket.org:

"Hello,

Thanks for your time and code. I will gladly merge the Vimeo InfoExtractor in youtube-dl provided some conditions are met. Most are dead simple, so here they go:

(1) You agree to release your code to the public domain, so it can be incorporated into youtube-dl without a license change.

(2) You agree to provide support for Vimeo issues in the issue tracker, and fix bugs related to that.

(3) You change your code slightly. I have found some minor problems:

(3.1) The URL regexp needs to accept "www." too. (Simply add "(?:www.)?" to the regular expression).

(3.2) The xml module you're using was not added until Python 2.5. While this is indeed perfectly reasonable, I'd like you to use the more unelegant regexp method seen in other InfoExtractors to maintain compatibility with Python 2.4.

Other than that, very good job! I tested it and it worked fine with a couple of videos. Issue another pull request when the changes are ready.

PS: If you had used import xml.etree.ElementTree at the top, you wouldn't need to use the "from xml..." line that can be seen inside the Vimeo InfoExtractor."

@rg3
Copy link
Collaborator

rg3 commented Nov 6, 2010

Ok. As I see and you said, it still has some changes pending. I'll leave the pull request open but don't forget to comment or email me when it's ready to merge, and I'll do.

Thanks for the time and code.

@canavan
Copy link

canavan commented Nov 20, 2010

I'd like to see support to switch to /sd/ from /hd/, e.g. with the -f command line option.

@rbrito
Copy link
Contributor

rbrito commented Nov 25, 2010

Hi there.

I didn't know that this was already implemented and I implemented an extractor in a fork. I don't consider it ready for prime time, but it may serve as inspiration.

In particular, I think that some of what I use is simpler than the proposed solution, as I am not using anything else than simple regexps. You may want to see it as my commit: rbrito/youtube-dl@c3ab017cb

Feel free to steal anything from here. (Yes, public domain).

Regards, Rogério Brito.

@rg3
Copy link
Collaborator

rg3 commented Nov 25, 2010

Hi!

rbrito, if your version supports choosing between SD and HD with the -f switch, or you think you can easily add it, please issue a pull request when ready, and I'll merge your changes.

@ppawel
Copy link
Author

ppawel commented Nov 26, 2010

rbrito, your version indeed looks simpler in terms of dependencies (no xml module needed) so I would recommend it over mine.

@FelixChery FelixChery mentioned this pull request Apr 24, 2019
joedborg referenced this pull request in joedborg/youtube-dl Nov 17, 2020
[pull] master from rg3:master
tsukumijima pushed a commit to tsukumijima/youtube-dl that referenced this pull request Dec 2, 2020
tsukumijima pushed a commit to tsukumijima/youtube-dl that referenced this pull request Dec 2, 2020
This pull request was closed.
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.

4 participants