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

Clarify extractors returning None #9963

Open
dstftw opened this issue Jun 30, 2016 · 6 comments
Open

Clarify extractors returning None #9963

dstftw opened this issue Jun 30, 2016 · 6 comments

Comments

@dstftw
Copy link
Collaborator

@dstftw dstftw commented Jun 30, 2016

What is the purpose of your issue?

  • Question

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:

  • none?
dstftw referenced this issue Jun 30, 2016
@yan12125
Copy link
Collaborator

@yan12125 yan12125 commented Jun 30, 2016

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
Copy link
Collaborator

@remitamine remitamine commented Jun 30, 2016

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
Copy link
Collaborator Author

@dstftw dstftw commented Jun 30, 2016

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
Copy link
Collaborator

@yan12125 yan12125 commented Jun 30, 2016

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

@dstftw
Copy link
Collaborator Author

@dstftw dstftw commented Jun 30, 2016

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
Copy link
Collaborator

@yan12125 yan12125 commented Jul 1, 2016

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().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.