Feature/multiple downloads #2
Conversation
Conflicts: vidscraper/tests/unit/test_vimeo.py vidscraper/tests/unit/test_youtube.py
|
As per http://bugzilla.pculture.org/show_bug.cgi?id=16718, it might make sense to have file_url_* properties return the favored download rather than the first one. Or alternatively, cache the favored download elsewhere and remove the file_url_* properties. I know, I was the one who suggested they be used as you're now using them, but it might make more sense to just drop them. Do we need them any more? |
Conflicts: vidscraper/suites/__init__.py vidscraper/suites/base.py vidscraper/suites/feed.py vidscraper/suites/vimeo.py vidscraper/suites/youtube.py vidscraper/tests/unit/test_generic.py vidscraper/tests/unit/test_vimeo.py vidscraper/tests/unit/test_youtube.py
Conflicts: vidscraper/tests/integration/test_youtube.py vidscraper/videos.py
Also removed file_url_* properties, cleaned up feedparser utils, and spread use of VideoFile to the blip and generic suites.
|
Remaining issues on this pull request:
|
|
Also, turns out that assertGreater (used in some updated test cases in this branch) requires unittest2, available natively in 2.7 but not 2.6. I'd be fine with introducing unittest2 as a dependency - or we could use nose's assertions. |
|
I believe that resolves all the issues I mentioned as remaining on this pull request. Paul: Would you be up for/interested in reviewing the changes? |
| @@ -41,3 +41,6 @@ | |||
| :func:`vidscraper.handles_feed`, respectively. They now accept the same | |||
| parameters as :func:`vidscraper.auto_scrape` and | |||
| :func:`vidscraper.auto_feed`. | |||
| * Multiple :class:`VideoFile`\ s are now made available for :class:`.Video` | |||
There was a problem hiding this comment.
Just to make sure I'm on the same page as you - the missing . before VideoFile and later before Video? Something about the "made available" wording?
There was a problem hiding this comment.
That just looks like a strange line, but I guess as long as it renders correctly then it's okay.
There was a problem hiding this comment.
It renders as intended. Fixed the missing . with b019289.
|
I'm not thrilled with the decision to totally drop |
|
Thanks for looking over this. The reasoning is that there should be one way and only one way to get at the first file. I also am not entirely convinced of the usefulness of being able to get at the first file as opposed to being able to get at the preferred file. I've been thinking of rolling bug 17716 and bug 16718 into 0.6 as a Is there any reason to keep it around as a property that I'm not seeing? |
|
The reason to keep it around would be so that a point upgrade to Vidscraper doesn't totally break people who are using it. But, IIRC there are a bunch of other backwards-incompatible changes in 0.6 so just documenting the change, and what the new code should look like, is okay. |
|
Opened a ticket for documenting backwards-incompatibilities. We do have the release notes, but more targeted documentation would be better. |
|
I believe I've addressed all comments with either a commit or an explanation. Let me know if there are any outstanding or continued concerns. I'll merge this on Wednesday barring further comment. |
No rush for merging this, but it is needed for enterprise C.