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

6720: Pressing enter uses default format #7101

Merged
merged 7 commits into from May 29, 2023

Conversation

ivanskodje
Copy link
Contributor

@ivanskodje ivanskodje commented May 22, 2023

IMPORTANT: PRs without the template will be CLOSED

Description of your pull request and other information

Original Issue Description:

Using -f- prompts "Enter format selector:"
When entering nothing (i.e. just pressing Enter) outputs "ERROR: Requested format is not available"

It would be more helpful in this case to autoselect the default format (i.e. the format which would have been selected
by not using the -f switch at all) instead of asking again (which doesn't make too much sense IMO).

  • Pressing enter will use default format.
  • Updated the wording of the message

Fixes #6720

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 [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 yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • 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?

Copilot Summary

πŸ€– Generated by Copilot at 7239233

Summary

πŸ†•πŸŽ›οΈπŸ“

Add default format spec option and debug message to interactive format selection mode in yt_dlp/YoutubeDL.py

format selection
press ENTER or CTRL+C
autumn leaves falling

Walkthrough

  • Add a feature to the interactive format selection mode that allows using the default format spec or quitting with keyboard shortcuts (link)
  • Display a debug message with the default format spec used in the interactive mode (link)

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

Let me know what you think of the changes

@ivanskodje
Copy link
Contributor Author

ivanskodje commented May 22, 2023

Let me know what you think of the changes

Moving of the format_selector is None looks fine, but the req_format change makes the code more difficult to read and understand.

Easy to read and understand:

req_format = input(self._format_screen('\nEnter format selector (press ENTER for default, or CTRL+C to quit): ', self.Styles.EMPHASIS))

Not so easy to read and understand:

req_format = input(' '.join((
                    self._format_screen('\nEnter format selector', self.Styles.EMPHASIS),
                    '(Press ENTER for default, or Ctrl+C to quit)',
                    self._format_screen(': ', self.Styles.EMPHASIS))
                ))

Does using join on an empty string with a space, and using two _format_screen() give benefits in Python that I am not aware of?

If the line was too long, we could separate the long text into a helper variable, but it was probably is fine the way it was.

prompt = '\nEnter format selector (press ENTER for default, or CTRL+C to quit): '
req_format = input(self._format_screen(prompt, self.Styles.EMPHASIS))

@Grub4K
Copy link
Member

Grub4K commented May 22, 2023

Take note that we are applying emphasis to just the parts that are not in the braces. We deliberately use str.join to take emphasis away from the braced part. I think the hanging brace should go into the above line though

@ivanskodje
Copy link
Contributor Author

Take note that we are applying emphasis to just the parts that are not in the braces. We deliberately use str.join to take emphasis away from the braced part. I think the hanging brace should go into the above line though

I am afraid I do not understand what you mean by "(...) take emphasis away from the braced part". Could you elaborate on what you mean? Is this a specific coding convention, or is there a functional purpose to this?

I agree regarding hanging brace, I recall reading the contribution docs about that avoiding hanging paranthesis. πŸ‘

@Grub4K
Copy link
Member

Grub4K commented May 22, 2023

We are calling self._format_screen() on only '\nEnter format selector' and ': ', making them bold. The braced part will be left regular. The line will render as


Enter format selector (Press ENTER for default, or Ctrl+C to quit) : _

I think we should go one further and use ''.join instead since I also don't like the space after the brace (to quit) : ), thinking about it now

@ivanskodje
Copy link
Contributor Author

Excellent feedback!
Did some cleanup, now the text should look the way we want it too 🀞

image

@Grub4K
Copy link
Member

Grub4K commented May 22, 2023

Extracting the emphasis into a function is overkill. Having to write the style to format with does not make it more unreadable imo. If thats reverted it LGTM

@ivanskodje
Copy link
Contributor Author

Extracting the emphasis into a function is overkill. Having to write the style to format with does not make it more unreadable imo. If thats reverted it LGTM

While I appreciate your viewpoint and will duly revert the changes, I have a suggestion that could enhance our coding practices moving forward.

To maintain a clean codebase and ensure easier code maintenance, I propose we adopt a strategy of separating concerns (YoutubeDL.py appears to do a little of everything). Specifically, we could introduce a dedicated class and corresponding functions to manage formatting tasks. This approach would mitigate the reliance on a multipurpose function, which can potentially augment complexity and demand a higher level of knowledge for utilization.

In the context of the function in question, its use cases appear to be quite limited, with only two observable uses: one involving FILENAME, and a few instances with self.Styles.EMPHASIS. These observations further endorse the implementation of dedicated functions for each distinct task. 😺

@Grub4K
Copy link
Member

Grub4K commented May 22, 2023

Thats already semi in progress. There are two big networking PRs, partially getting merged bit by bit into the codebase, there is #5680 doing the formatting and output separation. It just takes time and is a chore since the reworks are big and reviews take time, and we need to keep compat (somewhat) on the side...

@pukkandan pukkandan force-pushed the master branch 2 times, most recently from ee280c7 to 7aeda6c Compare May 24, 2023 18:09
@pukkandan pukkandan merged commit 372a0f3 into yt-dlp:master May 29, 2023
11 checks passed
@ivanskodje ivanskodje deleted the 6720-autoselet-format branch May 30, 2023 10:46
aalsuwaidi pushed a commit to aalsuwaidi/yt-dlp that referenced this pull request Apr 21, 2024
Authored by: ivanskodje, pukkandan
Closes yt-dlp#6720
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.

-f- + enter = autoselect format
3 participants