-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
base: master
Are you sure you want to change the base?
Conversation
a5130a6
to
112d5ce
Compare
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. |
112d5ce
to
76eab55
Compare
There was a problem hiding this 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
- a simplified version of
utils.supports_terminal_sequences()
- a
YoutubeDL._format_err()
method like that in yt-dlp, but self-contained - 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.
youtube_dl/YoutubeDL.py
Outdated
prefix = 'WARNING:' | ||
if self.can_use_color_codes(output_file=self._err_file): | ||
prefix = '\033[0;33m' + prefix + '\033[0m' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
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:') |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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:
- the same code pattern in
InfoExtractor._search_regex()
should be addressed (and also that this looks atsys.stderr
instead ofself._downloader._err_file
, probably wrong) - 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, defaultFalse
, 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.
There was a problem hiding this comment.
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.)
Please follow the guide below
x
into all the boxes [ ] relevant to your pull request (like that [x])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:
What is the purpose of your pull request?
Description of your pull request and other information
I think this way it's easier to read, and thus, easier to maintain.