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

Feat: add color to '* has already been recorded in the archive' message #5138

Merged
merged 8 commits into from Jul 15, 2023

Conversation

aaruni96
Copy link
Contributor

@aaruni96 aaruni96 commented Oct 4, 2022

Adds color to the message '* has already been recorded in the archive'

This PR adds color to the info line if a video has already been recorded in the arvhice.

Closes #4913

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp 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?

@Grub4K
Copy link
Member

Grub4K commented Oct 4, 2022

There are two occurences for the "recorded" message, one in _match_entry and one in extract_info.
Using f-strings for non legacy is prefered. Make sure to follow the contribution guidelines.
I am also not sure, but the key parameter in the to_screen should probably not have color, as that is part of the standardized output.

It makes sense to adjust the output to give the id and title if available, else only one of the two.

@Grub4K Grub4K added the enhancement New feature or request label Oct 4, 2022
@pukkandan
Copy link
Member

In addition, reason should not be colored

@pukkandan pukkandan added the pending-fixes PR has had changes requested label Oct 4, 2022
@pukkandan pukkandan removed the pending-fixes PR has had changes requested label Oct 5, 2022
@aaruni96
Copy link
Contributor Author

aaruni96 commented Oct 5, 2022

If there is no more work in this PR, but it takes time to merge, can you in the meantime tag this with hacktoberfest-accepted ?

yt_dlp/YoutubeDL.py Outdated Show resolved Hide resolved
yt_dlp/YoutubeDL.py Outdated Show resolved Hide resolved
@pukkandan pukkandan added the pending-fixes PR has had changes requested label Nov 11, 2022
@pukkandan pukkandan added the stale-pr PR that has been pending fixes for a long time label Dec 27, 2022
@pukkandan pukkandan removed pending-fixes PR has had changes requested stale-pr PR that has been pending fixes for a long time labels Jan 3, 2023
@pukkandan pukkandan force-pushed the master branch 2 times, most recently from ee280c7 to 7aeda6c Compare May 24, 2023 18:09
@johndrinkwater
Copy link

Came to report youtube playlist download mode doesn’t list video IDs of already archived links compared to regular youtube urls, and luckily found this patches that.

Was this PR waiting for further updates?

Copy link
Member

@Grub4K Grub4K left a comment

Choose a reason for hiding this comment

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

This would be what I thought of

Comment on lines 1354 to 1355
video_id = info_dict.get('id', 'entry')
video_title = info_dict.get('title', video_id)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of defaulting to video_id, id use format_field to only show if a good value is contained:

Suggested change
video_id = info_dict.get('id', 'entry')
video_title = info_dict.get('title', video_id)
video_id = info_dict.get('id', 'entry')
video_title = format_field(info_dict, 'title', ' (%s)', func=lambda x: self._format_screen(x, self.Styles.FILENAME))

Copy link
Member

Choose a reason for hiding this comment

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

video_title is used elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

Oh my bad, so something like title_format = ...?

yt_dlp/YoutubeDL.py Outdated Show resolved Hide resolved
yt_dlp/YoutubeDL.py Outdated Show resolved Hide resolved
@pukkandan
Copy link
Member

pukkandan commented Jul 15, 2023

Came to report youtube playlist download mode doesn’t list video IDs of already archived links compared to regular youtube urls, and luckily found this patches that.

Was this PR waiting for further updates?

Whatever your issue is, it does not seem related to this PR.

@pukkandan
Copy link
Member

Ive pushed something. Let me know opinion.

@Grub4K
Copy link
Member

Grub4K commented Jul 15, 2023

I would have wanted a clearer separation for the title, but this is way cleaner code LGTM.

@pukkandan
Copy link
Member

Hm.. match_filter messages are inconsistent whether it puts title in "" or not. We can standardize it later

@Grub4K
Copy link
Member

Grub4K commented Jul 15, 2023

I would always put title in quotes, but yes, we can standardize later, maybe in conjunction with structural logging.

@pukkandan pukkandan merged commit 2b029ca into yt-dlp:master Jul 15, 2023
13 checks passed
aalsuwaidi pushed a commit to aalsuwaidi/yt-dlp that referenced this pull request Apr 21, 2024
Authored by: aaruni96, Grub4K, pukkandan
Closes yt-dlp#4913
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add some color to '* has already been recorded in the archive' message to highlight output of download entry
4 participants