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

[franceculture] Fix extractor #27903

Merged
merged 2 commits into from
Jan 22, 2021
Merged

[franceculture] Fix extractor #27903

merged 2 commits into from
Jan 22, 2021

Conversation

aurelg
Copy link

@aurelg aurelg commented Jan 21, 2021

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly
  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])
  • Use Preview tab to see how your pull request will actually look like

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:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

  • Bug fix
  • Improvement
  • New extractor
  • New feature

Description of your pull request and other information

Explanation of your pull request in arbitrary form goes here. Please make sure the description explains the purpose and effect of your pull request and is worded well enough to be understood. Provide as much context and examples as possible.

Thanks for this wonderful work!

youtube_dl/extractor/franceculture.py Outdated Show resolved Hide resolved
youtube_dl/extractor/franceculture.py Outdated Show resolved Hide resolved
youtube_dl/extractor/franceculture.py Outdated Show resolved Hide resolved
youtube_dl/extractor/franceculture.py Outdated Show resolved Hide resolved
@aurelg aurelg requested a review from dstftw January 22, 2021 07:57
@@ -36,12 +36,12 @@ def _real_extract(self, url):
</h1>|
<div[^>]+class="[^"]*?(?:title-zone-diffusion|heading-zone-(?:wrapper|player-button))[^"]*?"[^>]*>
).*?
(<button[^>]+data-asset-source="[^"]+"[^>]+>)
(<button[^>]+data-(asset-source|url)="[^"]+"[^>]+>)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't capture groups you don't use.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replace data-(asset-source|url) by data-(?:asset-source|url) in the next commit. Is that what you expect?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

''',
webpage, 'video data'))

video_url = video_data['data-asset-source']
title = video_data.get('data-asset-title') or self._og_search_title(webpage)
video_url = video_data.get('data-asset-source') or video_data.get('data-url')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Video URL is mandatory. Read coding conventions.

Copy link
Author

@aurelg aurelg Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your help, that's my first contribution to this project and I'm still a bit lost after reading the coding conventions.

As far as I know, either data-asset-source or data-url can be found in the HTML, on the previous and current versions of the FranceCulture website, respectively. This is the root cause of the current extractor being broken, and the very reason of this PR. If both are missing, though, video_url will be False and I understand that's inappropriate and the need for a default value. Yet, I can't figure out what to use since none seem to make sense as the extractor will fail anyway.

What would be an acceptable solution?

@dstftw dstftw merged commit d8dab85 into ytdl-org:master Jan 22, 2021
ThirumalaiK pushed a commit to ThirumalaiK/youtube-dl that referenced this pull request Jan 28, 2021
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.

2 participants