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

yt-dlp: update page #12580

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

yt-dlp: update page #12580

wants to merge 22 commits into from

Conversation

thrila
Copy link

@thrila thrila commented Mar 30, 2024

  • The page(s) are in the correct platform directories: common, linux, osx, windows, sunos, android, etc.
  • The page(s) have at most 8 examples.
  • The page description(s) have links to documentation or a homepage.
  • The page(s) follow the content guidelines.
  • The PR title conforms to the recommended templates.
  • Version of the command being documented (if known):

@github-actions github-actions bot added the page edit Changes to an existing page(s). label Mar 30, 2024
@thrila
Copy link
Author

thrila commented Mar 30, 2024

@kbdharun please confirm

pages/common/yt-dlp.md Outdated Show resolved Hide resolved
pages/common/yt-dlp.md Outdated Show resolved Hide resolved
pages/common/yt-dlp.md Outdated Show resolved Hide resolved
pages/common/yt-dlp.md Outdated Show resolved Hide resolved
@thrila
Copy link
Author

thrila commented Mar 30, 2024

Thanks for the correction

pages/common/yt-dlp.md Outdated Show resolved Hide resolved
Magrid0
Magrid0 previously approved these changes Mar 30, 2024
Copy link
Member

@Magrid0 Magrid0 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@kbdharun kbdharun changed the title chore: add page for yt-dlp yt-dlp: update page Mar 30, 2024
pages/common/yt-dlp.md Outdated Show resolved Hide resolved
@CleanMachine1
Copy link
Member

You did not make any changes following my comments, and then proceeded to close them, saying they have been resolved.

Please actually respond to them appropriately.

Copy link
Member

@CleanMachine1 CleanMachine1 left a comment

Choose a reason for hiding this comment

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

Please do not approve before actually reading and commenting on my original feedback.

@thrila
Copy link
Author

thrila commented Apr 2, 2024

my apologies started using github mobile seems im yet to fully understand it , still i thought the change you made was changing the placeholder url to the keyword url

@CleanMachine1
Copy link
Member

Do you plan on changing anything suggested, or should we close this PR?

@thrila
Copy link
Author

thrila commented Apr 13, 2024

Do you plan on changing anything suggested, or should we close this PR?

yeah lemme do it now kinda forgot

@thrila thrila closed this Apr 13, 2024
@thrila thrila reopened this Apr 13, 2024
@thrila
Copy link
Author

thrila commented Apr 13, 2024

Do you plan on changing anything suggested, or should we close this PR?

Hi finished do you mind checking?

@CleanMachine1
Copy link
Member

That commit has nothing in it, did you not push your changes?

@thrila
Copy link
Author

thrila commented Apr 17, 2024

That commit has nothing in it, did you not push your changes?

Seems i didnt my bad haven't really contributed to a project before Thanks for the patience

pages/common/yt-dlp.md Outdated Show resolved Hide resolved
@thrila
Copy link
Author

thrila commented May 5, 2024

Any more corrections?

@vitorhcl
Copy link
Member

vitorhcl commented May 6, 2024

I think he is just merging the branch instead of commit the changes.

@thrila if you are on GitHub Mobile, you meed to click on the comments and then click om the "Commit" button.

@thrila
Copy link
Author

thrila commented May 6, 2024

I am on mobile thanks for the tip
Still there's one thing that confuses me
As you can see from the last stream of commits the placeholder was removed and a example URL was placed I don't know if this is something he did for all of them how do I check? Other than that I actually started with an example URL and some dudes said a place holder was better when I made a PR

@vitorhcl
Copy link
Member

vitorhcl commented May 7, 2024

Still there's one thing that confuses me
As you can see from the last stream of commits the placeholder was removed and a example URL was placed

I saw the last commits and it looks like it's the opposite, you replaced the example URLs with the generic placeholder text "url".

I don't know if this is something he did for all of them how do I check?

Well, the current state is replacing some examples URLs with the "url" placeholder.

Anyways, you can see the changes that would be applied if thisnPR was merged clicking on the first bar below the PR description that has green and red numbers (the quantity of modified lines).

Green = addition, red = removal.

Other than that I actually started with an example URL and some dudes said a place holder was better when I made a PR

Are you talking about #12580 (comment)? I don't know if @kbdharun thinks it is a good idea or if that was just a suggestion for improving the page consistency.

In my opinion, in this case, example URLs are more user-friendly.

@kbdharun
Copy link
Member

kbdharun commented May 9, 2024

Are you talking about #12580 (comment)? I don't know if @kbdharun thinks it is a good idea or if that was just a suggestion for improving the page consistency.

Agreed, having example URLs seems much better. I wasn't clear in my previous review comment, what I meant is since one example was updated to the URL I suggested updating it throughout the page or reverting the link in the specific line (in this case, reverting seems like the best course of action).

@sebastiaanspeck
Copy link
Member

Hi @thrila, we are sending you a friendly nudge this PR still has unresolved comments. Could you please make the suggested changes?

@thrila
Copy link
Author

thrila commented May 26, 2024

Hi @thrila, we are sending you a friendly nudge this PR still has unresolved comments. Could you please make the suggested changes?

yeah will do it right now

@thrila thrila requested a review from spageektti as a code owner May 26, 2024 01:25
@sebastiaanspeck sebastiaanspeck mentioned this pull request May 26, 2024
5 tasks
thrila and others added 2 commits May 27, 2024 08:58
@thrila
Copy link
Author

thrila commented May 27, 2024

done !! please check to make sure it's the format we want

Copy link
Member

@kbdharun kbdharun 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 contribution. I have some suggestions for this page. After this the PR is GTG.

pages/common/yt-dlp.md Outdated Show resolved Hide resolved
pages/common/yt-dlp.md Outdated Show resolved Hide resolved
pages/common/yt-dlp.md Outdated Show resolved Hide resolved
pages/common/yt-dlp.md Outdated Show resolved Hide resolved
pages/common/yt-dlp.md Outdated Show resolved Hide resolved
pages/common/yt-dlp.md Outdated Show resolved Hide resolved
@@ -27,12 +27,12 @@

- Download all playlists of YouTube channel/user keeping each playlist in separate directory:

`yt-dlp -o "{{%(uploader)s/%(playlist)s/%(playlist_index)s - %(title)s.%(ext)s}}" "{{https://www.youtube.com/user/TheLinuxFoundation/playlists}}"`
`yt-dlp -o "{{%(uploader)s/%(playlist)s/%(playlist_index)s - %(title)s.%(ext)s}}" "{{https://www.youtube.com/watch?v=oHg5SJYRHA0}}"`
Copy link
Member

Choose a reason for hiding this comment

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

IMO this example URL does not match the description above

Copy link
Member

Choose a reason for hiding this comment

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

Hi, would you please not resolve a comment without any explanation why or not changing anything.

@thrila
Copy link
Author

thrila commented May 27, 2024

Thanks for your contribution. I have some suggestions for this page. After this the PR is GTG.

Well ok I'll make the changes

thrila and others added 7 commits May 27, 2024 17:21
Co-authored-by: K.B.Dharun Krishna <kbdharunkrishna@gmail.com>
Co-authored-by: K.B.Dharun Krishna <kbdharunkrishna@gmail.com>
Co-authored-by: K.B.Dharun Krishna <kbdharunkrishna@gmail.com>
Co-authored-by: K.B.Dharun Krishna <kbdharunkrishna@gmail.com>
Co-authored-by: K.B.Dharun Krishna <kbdharunkrishna@gmail.com>
Co-authored-by: K.B.Dharun Krishna <kbdharunkrishna@gmail.com>
@thrila
Copy link
Author

thrila commented May 27, 2024

can some check to connfirm that the changes were indeed applied?

Copy link
Member

@kbdharun kbdharun left a comment

Choose a reason for hiding this comment

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

LGTM, as the other maintainers say these links must be updated to align with their example description.


`yt-dlp -o "{{%(uploader)s/%(playlist)s/%(playlist_index)s - %(title)s.%(ext)s}}" "{{https://www.youtube.com/user/TheLinuxFoundation/playlists}}"`
`yt-dlp -o "{{%(uploader)s/%(playlist)s/%(playlist_index)s - %(title)s.%(ext)s}}" "{{https://www.youtube.com/watch?v=oHg5SJYRHA0}}"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`yt-dlp -o "{{%(uploader)s/%(playlist)s/%(playlist_index)s - %(title)s.%(ext)s}}" "{{https://www.youtube.com/watch?v=oHg5SJYRHA0}}"`
`yt-dlp -o "{{%(uploader)s/%(playlist)s/%(playlist_index)s - %(title)s.%(ext)s}}" "{{https://www.youtube.com/user/TheLinuxFoundation/playlists}}"`


`yt-dlp -P "{{C:/MyVideos}}" -o "{{%(series)s/%(season_number)s - %(season)s/%(episode_number)s - %(episode)s.%(ext)s}}" "{{https://videomore.ru/kino_v_detalayah/5_sezon/367617}}"`
`yt-dlp -P "{{path/to/directory}}" -o "{{%(series)s/%(season_number)s - %(season)s/%(episode_number)s - %(episode)s.%(ext)s}}" "{{https://www.youtube.com/watch?v=oHg5SJYRHA0}}"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`yt-dlp -P "{{path/to/directory}}" -o "{{%(series)s/%(season_number)s - %(season)s/%(episode_number)s - %(episode)s.%(ext)s}}" "{{https://www.youtube.com/watch?v=oHg5SJYRHA0}}"`
`yt-dlp -P "{{path/to/directory}}" -o "{{%(series)s/%(season_number)s - %(season)s/%(episode_number)s - %(episode)s.%(ext)s}}" "{{https://videomore.ru/kino_v_detalayah/5_sezon/367617}}"`

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this URL valid because I got 403?

Copy link
Member

Choose a reason for hiding this comment

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

Is this URL valid because I got 403?

Oh, I didn't check for it (just reverted the change), do you have any suggestions for a new one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this URL valid because I got 403?

Oh, I didn't check for it (just reverted the change), do you have any suggestions for a new one?

No, because I don't understand this example fully.

Copy link
Member

Choose a reason for hiding this comment

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

@CleanMachine1 CleanMachine1 removed their request for review May 28, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
page edit Changes to an existing page(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants