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.

Sign up for GitHub

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
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions youtube_dl/YoutubeDL.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.)

"""Decide if we should use color codes for the given output channel."""
# Try to keep criteria ordered by computational effort, easiest first.
if compat_os_name == 'nt':
return False
if self.params.get('no_color'):
return False
if output_file is not None:
if not output_file.isatty():
return False
return True

def report_warning(self, message, only_once=False, _cache={}):
'''
Print the message to stderr, it will be prefixed with 'WARNING:'
Expand All @@ -637,17 +649,18 @@ def report_warning(self, message, only_once=False, _cache={}):
if m_cnt > 0:
return

if self.params.get('logger') is not None:
self.params['logger'].warning(message)
else:
if self.params.get('no_warnings'):
return
if not self.params.get('no_color') and self._err_file.isatty() and compat_os_name != 'nt':
_msg_header = '\033[0;33mWARNING:\033[0m'
else:
_msg_header = 'WARNING:'
warning_message = '%s %s' % (_msg_header, message)
self.to_stderr(warning_message)
custom_logger = self.params.get('logger')
if custom_logger is not None:
custom_logger.warning(message)
return

if self.params.get('no_warnings'):
return

prefix = 'WARNING:'
if self.__can_use_color_codes(output_file=self._err_file):
prefix = '\033[0;33m' + prefix + '\033[0m'
self.to_stderr(prefix + ' ' + message)

def report_error(self, message, *args, **kwargs):
'''
Expand Down
Loading