ytdl-org / youtube-dl

Command-line program to download videos from YouTube.com and other video sites
http://ytdl-org.github.io/youtube-dl/
The Unlicense
131.9k stars 10k forks source link

Clarify extractors returning None #9963

Open dstftw opened 8 years ago

dstftw commented 8 years ago

What is the purpose of your issue?


There are some extractors in youtube-dl's codebase that may return None resulting in youtube-dl exiting silently. This pattern is used for a long time already even in non-legacy code. InfoExtractor is sort of ambiguous about returning None and thus should be clarified. As such we can allow returning None from extractor and print generic no-videos-found error message (or raise exception). Or we can forbid this at all and always require extractor to return a standard info dict. What are the opinions?

Extractors that may technically return None:

We may also finally drop compat_list compatibility code since we don't have such extractors anymore (or do we?).

compat_list extractors:

yan12125 commented 8 years ago

A vote for printing generic no-videos-found error messages. This simplifies error handling in individual extractors and avoid possible duplicated/similar codes.

We may also finally drop compat_list compatibility code since we don't have such extractors anymore

YoutubeDL reports a warning at https://github.com/rg3/youtube-dl/blob/master/youtube_dl/YoutubeDL.py#L854-L856. If there's a working extractor that returns a compat_list, there's an error in tests. I see no one in a recent test, so it's safe to remove it.

remitamine commented 8 years ago

previously muliple members told me that i should raise an error if there are no formats to extract(at least here and here). but i prefer this solution:

As such we can allow returning None from extractor and print generic no-videos-found error message (or raise exception).

instead of repeat the same thing in every extractor just handle it once.

dstftw commented 8 years ago

YoutubeDL reports a warning at https://github.com/rg3/youtube-dl/blob/master/youtube_dl/YoutubeDL.py#L854-L856. If there's a working extractor that returns a compat_list, there's an error in tests. I see no one in a recent test, so it's safe to remove it.

It does not take skipped tests into account so there still may be such extractors.

yan12125 commented 8 years ago

You're right. Checking all extractors is the only way.

dstftw commented 8 years ago

I'd also prefer a generic error since it offers explicit indication at the same time providing a room for customization (particular extractor may throw own ExtractorError if not satisfied with the generic one). The only problem I see with this approach is that it's not possible to determine whether None was intentional by extractor or not. Or in other words, it's impossible to guess whether the message should be "No videos found" or "No videos found by extractor. It may be an error with extractor ...".

yan12125 commented 8 years ago

If an extractor is quite sure there are no videos, it can raise a different error, for example a new method named _raise_no_videos_error().