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

[QingTing] Add new extractor #31021

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

changren-wcr
Copy link

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

This pull request adds qingting.fm, probably the largest podcast website in China.

Copy link
Contributor

@dirkf dirkf left a comment

Choose a reason for hiding this comment

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

Thanks for your work.

I'm not sure how this extraction is meant to work.

  • I can play a link from the site like https://www.qingting.fm/channels/214008/programs/7016167/ in the UK but the .../vchannels/... links just give a page whose source is OK, with no actual HTML, JS or whatever.
  • The /channels/ page tries for a URL with .mp3 that got 504 Gateway Timeout; the actual audio played seems to be m4a.
  • Where would a user find a link like the test URL? I only see /channels/ links on the site, and surely those need to be handled? If there is a simple correspondence you can have an IE class for each one and direct from one to the other using url_result().

Assuming something might work somehow I've commented on the existing code anyway.

youtube_dl/extractor/qingting.py Outdated Show resolved Hide resolved
youtube_dl/extractor/qingting.py Outdated Show resolved Hide resolved
youtube_dl/extractor/qingting.py Outdated Show resolved Hide resolved
youtube_dl/extractor/qingting.py Outdated Show resolved Hide resolved
youtube_dl/extractor/qingting.py Outdated Show resolved Hide resolved
youtube_dl/extractor/qingting.py Outdated Show resolved Hide resolved
changren-wcr and others added 5 commits June 16, 2022 19:02
Co-authored-by: dirkf <fieldhouse@gmx.net>
Co-authored-by: dirkf <fieldhouse@gmx.net>
_html_search_regex() has default fatal=True: add a default to fall back to _og_search_title()

allow line break in .*

Co-authored-by: dirkf <fieldhouse@gmx.net>
Co-authored-by: dirkf <fieldhouse@gmx.net>
@changren-wcr
Copy link
Author

changren-wcr commented Jun 16, 2022

There are two types of URLs, desktop url and mobile url. The corresponding mobile url of desktop url https://www.qingting.fm/channels/214008/programs/7016167/ is https://m.qtfm.cn/vchannels/214008/programs/7016167/.

A new extractor is added for desktop url, which will extract the alternate mobile url from webpage of desktop url.

@changren-wcr changren-wcr requested a review from dirkf June 16, 2022 13:01
@gamer191
Copy link

gamer191 commented Jun 17, 2022

This PR currently contains the following errors (thanks Dirkf for enabling the tests):

./youtube_dl/extractor/qingting.py:4:1: F401 're' imported but unused
./youtube_dl/extractor/qingting.py:10:1: E302 expected 2 blank lines, found 1
./youtube_dl/extractor/qingting.py:41:1: E302 expected 2 blank lines, found 1
AssertionError: 2 != 1 : Multiple extractors with the same IE_NAME "qingting" (QingTingMobileIE, QingTingDeskTopIE)

there might be more errors in this log, but I don't think so

@changren-wcr
Copy link
Author

This PR currently contains the following errors (thanks Dirkf for enabling the tests):

./youtube_dl/extractor/qingting.py:4:1: F401 're' imported but unused
./youtube_dl/extractor/qingting.py:10:1: E302 expected 2 blank lines, found 1
./youtube_dl/extractor/qingting.py:41:1: E302 expected 2 blank lines, found 1
AssertionError: 2 != 1 : Multiple extractors with the same IE_NAME "qingting" (QingTingMobileIE, QingTingDeskTopIE)

Thanks a lot! These errors have been fixed.

@changren-wcr
Copy link
Author

This is my first pull request on GitHub, please help me further refine my code and merge it into master. Thanks a lot!

@UnixCro
Copy link

UnixCro commented Jun 26, 2022

This is such an ugly website that we would only harm youtube-dl with it...

@dirkf
Copy link
Contributor

dirkf commented Jun 26, 2022

The rules for site support don't cover aesthetics of design or content!

@changren-wcr
Copy link
Author

The rules for site support don't cover aesthetics of design or content!

I really appreciate your patience and kindness in this PR, which is invaluable for a novice in the filed of open-source

@UnixCro
Copy link

UnixCro commented Jun 27, 2022

It is your repository and therefore your responsibility. I don't know anyone who has such an ugly website and the people who look at extractor.py out of curiosity that this website exists may be disturbed. I just wanted to help youtube-dl ultimately it's up to you what you think of me.

@dirkf
Copy link
Contributor

dirkf commented Jun 27, 2022

If there is test metadata that you think people might find unpleasant, by all means propose it to be changed to 'md5:....' format.

@gamer191
Copy link

gamer191 commented Jul 3, 2022

the people who look at extractor.py out of curiosity that this website exists may be disturbed

The golden rule: never look at a random website from extractors.py, as it will most likely be NSFW!

@UnixCro
Copy link

UnixCro commented Jul 3, 2022

We humans are extremely curious creatures by nature. If someone stops us from doing something, we will keep trying until we fall out of experience. I think it sucks that people accept pull requests like this. Getting more people into that strain by watching this content out of curiosity. The one who accepted the pull request made me angry because he can't think that far, but he always helped me with help, so I tend to keep my mouth shut, but I don't think it's good!

@rautamiekka
Copy link
Contributor

rautamiekka commented Jul 3, 2022

The golden rule: never look at a random website from extractors.py, as it will most likely be NSFW!

Like, I thought that's common sense when you work on/with an app that downloads from websites ? Well, any app whose source code contains any links, really, and being NSFW is pretty much irrelevant to the "don't click on random links if you dunno what you're doing" point.

@garoto
Copy link

garoto commented Jul 3, 2022

Where is the cuckoo reaction emoji when you need it.

@gamer191
Copy link

We humans are extremely curious creatures by nature. If someone stops us from doing something, we will keep trying until we fall out of experience. I think it sucks that people accept pull requests like this. Getting more people into that strain by watching this content out of curiosity. The one who accepted the pull request made me angry because he can't think that far, but he always helped me with help, so I tend to keep my mouth shut, but I don't think it's good!

@UnixCro Out of interest, what is on the website (I didn't open it for obvious reasons)?

I had assumed that you meant that you just didn't like that website, but if there is something actually bad on that website then I take back what I said (although obviously it's not my call because I'm not a maintainer)

@changren-wcr
Copy link
Author

changren-wcr commented Nov 13, 2022

The same pull request (yt-dlp/yt-dlp#5329) has already been merge in yt-dlp, which is a youtube-dl fork with additional features and fixes. @dirkf

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.

None yet

6 participants