-
Notifications
You must be signed in to change notification settings - Fork 10k
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
[heise] fix title extraction, modify test accordingly #15784
Conversation
youtube_dl/extractor/heise.py
Outdated
@@ -76,7 +74,9 @@ def _real_extract(self, url): | |||
if not title or title == "c't": | |||
title = self._search_regex( | |||
r'<div[^>]+class="videoplayerjw"[^>]+data-title="([^"]+)"', | |||
webpage, 'title') | |||
webpage, 'title', default=None, fatal=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default and fatal are not used together.
youtube_dl/extractor/heise.py
Outdated
webpage, 'title') | ||
webpage, 'title', default=None, fatal=False) | ||
if not title: | ||
self._og_search_title(webpage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has no effect.
Obviously you were right! Thanks a lot... ^^ |
youtube_dl/extractor/heise.py
Outdated
webpage, 'title') | ||
webpage, 'title', default=None) | ||
if not title: | ||
title = self._og_search_title(webpage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding another fallback here does not make much sense since title is not used when delegated to kaltura anyway.
Is this ready, or shall I change anything? Regards! |
title = self._search_regex( | ||
r'<div[^>]+class="videoplayerjw"[^>]+data-title="([^"]+)"', | ||
webpage, 'title') | ||
webpage, 'title', default=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean, "nothing changed"? If I use the version on master, the title extraction still fails for some videos (the latest c't uplink episode worked out just fine), especially the one in the unit test.
Regards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding another fallback here does not make much sense since title is not used when delegated to kaltura anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway as said, If i don't do so, some videos can't be downloaded, because the title extraction fails. Also, not all videos get directed to KalturaIE, I think (can't provide an example right now though).
Is there any way of giving the extracted title to KalturaIE?
Regards!
Before submitting a pull request make sure you have:
In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:
What is the purpose of your pull request?
Description of your pull request and other information
I fixed the title extraction for heise. All three unit tests now work (had to adjust one).
This fixes #15496 and also even makes my own old pull request #14108 obsolete and closable.
Accordingly, this also can replace #15026 (corrected wrong pull request number).