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

[vlive] merge youtube-dl and add new url support #248

Closed
wants to merge 2 commits into from
Closed

[vlive] merge youtube-dl and add new url support #248

wants to merge 2 commits into from

Conversation

kyuyeunk
Copy link
Contributor

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 solves vlive.py related problems mentioned in #245 (comment).

I've started with vlive.py code in current youtube_dl repository and improved it to support new url type (i.e., https://www.vlive.tv/post/<postId>).

The code determines if a url is old type or new type. And for fetching data, it uses post/v1.0/officialVideoPost-<videoId> for old type and post/v1.0/post-<postId> for new type.

Example of a new url is added in _TESTS.

@kyuyeunk
Copy link
Contributor Author

@pukkandan This pull request should maintain code similarity to youtube_dl while also supporting new url type.

@pukkandan
Copy link
Contributor

Wow, you work fast. Can you confirm if this closes #226, #221 and #222

Also, it makes #223, #224 and #227 obsolete, correct?

@kyuyeunk
Copy link
Contributor Author

@pukkandan No, this does not closes #226, #221, and #222 nor makes #223, #224, and #227 obsolete.

Above mentioned issues/bugs are present in both youtube-dl and youtube-dlc versions.

This pull request merely maintains similarity with youtube-dl codebase while implementing a feature that was already implemented in youtube-dlc version (i.e., new url support). Therefore, the issues are also present in this pull request as well.

Pull requests #223, #224, and #227 solves above mentioned issues, so these pull requests are needed.

I'm not familiar with the contribution guidelines of this repo, but should I try to merge above mentioned pull requests into this pull request? Or is it better to maintain a separate pull requests?

@pukkandan
Copy link
Contributor

I am also not sure. But if the earlier PRs can still be applied on top of this patch, I think it is best to keep the PRs seperate

@kyuyeunk
Copy link
Contributor Author

I am also not sure. But if the earlier PRs can still be applied on top of this patch, I think it is best to keep the PRs seperate

Thanks for the input. I will keep pull requests separate for now.

@pukkandan
Copy link
Contributor

Related: ytdl-org/youtube-dl@836c810

@kyuyeunk
Copy link
Contributor Author

Related: ytdl-org/youtube-dl@836c810

@pukkandan I can verify that the latest youtube-dl update makes this PR obsolete.
Also, it seems to fix issue #226 (making PR #227 obsolete).

I will close above mentioned PRs and issue when vlive.py changes of youtube-dl are incorporated into #245

However, issues #221 (PR #223) and #222 (PR #224) are still present.

@pukkandan
Copy link
Contributor

Is it sufficient to replace the entire vlive.py with dl's version?

@pukkandan
Copy link
Contributor

@kyuyeunk
Copy link
Contributor Author

Is it sufficient to replace the entire vlive.py with dl's version?

@pukkandan Yes it is sufficient.

38d7028#diff-b99f3b79e6a574f95b452073e111b94319a697d80ba7e62528556dfe0498b7ec

I can confirm that this commit works as intended and makes this PR obsolete.
Therefore, I will close this PR.

@kyuyeunk kyuyeunk closed this Nov 28, 2020
siikamiika pushed a commit to siikamiika/yt-dlc that referenced this pull request Jun 19, 2021
* Quote the `$0` variable to correctly handle spaces
* Change the shebang line to `/bin/sh` to avoid unnecessarily depending on bash
* Use the `exec` command to avoid having the shell process linger unnecessarily
* Change the mode to make the script directly executable

Authored by: fstirlitz

:ci skip all
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