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

[core] Simplify code for report_warning(). #32784

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mk-pmb
Copy link
Contributor

@mk-pmb mk-pmb commented May 15, 2024

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

I think this way it's easier to read, and thus, easier to maintain.

@mk-pmb
Copy link
Contributor Author

mk-pmb commented May 15, 2024

GitHub seems to not show me your question in the code view, so I have to reply here: I'm ok with the double low line prefix. If someone needs this method in another scope, they can PR it. I'll amend.

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 this.

There may be some weirdness in how things like report_warning() work because of back-porting yt-dlp changes to extractor/common.py and YoutubeDL.py separately. Also, there are overrides in the test code as well (eg, to turn off colours).

Overall, I think the best way to improve this is to have

  1. a simplified version of utils.supports_terminal_sequences()
  2. a YoutubeDL._format_err() method like that in yt-dlp, but self-contained
  3. a YoutubeDL.Styles object that encodes the ANSI terminal sequences.

So:

class YoutubeDL(object):
    ...
    Styles = collections.NamedTuple('Styles', ('EMPHASIS', 'WARNING'))(
        # ESC[0;34m  ESC[0m     ESC[0;33m   ESC[0m  
        '\033[0;34m%s\033[0m', '\033[0;33m%s\033[0m')
    ...
    def _format_err(self, text, f, *args, **kwargs):
        # ignoring: fallback=None, *, test_encoding=False
        if self.params.get('no_color') is True or not supports_terminal_sequences(self._err_file):
            return text
        return f % text
    ...

These changes would yield a subset of the equivalent yt-dlp code, making it easier to keep our code and theirs in sync.

Comment on lines 660 to 662
prefix = 'WARNING:'
if self.can_use_color_codes(output_file=self._err_file):
prefix = '\033[0;33m' + prefix + '\033[0m'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

Suggested change
prefix = 'WARNING:'
if self.can_use_color_codes(output_file=self._err_file):
prefix = '\033[0;33m' + prefix + '\033[0m'
prefix = (
'\033[0;33m{0}\033[0m'
if self.can_use_color_codes(output_file=self._err_file)
else '{0}').format('WARNING:')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels confusing to me. Why would you always call .format() when one of the branches is a guaranteed verbatim copy?

@@ -625,6 +625,18 @@ def trouble(self, *args, **kwargs):
raise DownloadError(message, exc_info)
self._download_retcode = 1

def __can_use_color_codes(self, output_file=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

This method only depends on the instance to provide the no_color param. One possibility is to make it a function in utils.py with the no_color test separate.

If it's going to be a method here as-is:

  1. the same code pattern in InfoExtractor._search_regex() should be addressed (and also that this looks at sys.stderr instead of self._downloader._err_file, probably wrong)
  2. if the method is going to be called there, the name should lose the __, contrary to my original suggestion.

yt-dlp has invented minicurses.py which abstracts all this (more than we need, I think), but also this useful utils function that we might acquire:

@functools.cache
def supports_terminal_sequences(stream):
    if compat_os_name == 'nt':
        if not WINDOWS_VT_MODE:
            return False
    elif not os.getenv('TERM'):
        return False
    try:
        return stream.isatty()
    except BaseException:
        return False

The cache decorator that isn't available in all our supported Pythons can be simulated (I have an implementation but the margin of this comment is too small to contain it). The noteworthy changes:

  • WINDOWS_VT_MODE is introduced, default False, that can be enabled by a Windows API call for recent Windows (>= 10.0.10586) -- we can ignore that
  • the TERM env var is tested, which we don't do, and perhaps should
  • the isatty() call is protected, probably sensible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That exceeds my current available mental energy, so I have no opinion. If you need any changes on my side, I need dumber instructions, or you could probably edit it yourself. (The "Allow edits … by maintainers" is checked.)

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