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
Clean up terminal width handling #9143
Conversation
This was added by 91adf61, yet colorize() doesn't actually have a fixed_terminal_width argument.
Those two callers are the only two, and both check MYPY_FORCE_TERMINAL_WIDTH before calling get_terminal_width().
The documentation for Python's os.get_terminal_size() says: shutil.get_terminal_size() is the high-level function which should normally be used, os.get_terminal_size is the low-level implementation. https://docs.python.org/3/library/os.html#os.get_terminal_size shutil.get_terminal_size() already takes care of various corner cases such as: - Providing a fallback (to 80 columns, like the current DEFAULT_COLUMNS) - Returning that fallback if os.get_terminal_size() raises OSError - Doing the same if it returns a size of 0 In addition to that: - It takes care of some more corner cases ("stdout is None, closed, detached, or not a terminal, or os.get_terminal_size() is unsupported") - It adds support for the COLUMNS environment variable (rather than the non-standard MYPY_FORCE_TERMINAL_WIDTH) https://github.com/python/cpython/blob/v3.8.3/Lib/shutil.py#L1298-L1341
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.
LGTM
this regressed 9dcccca |
@asottile Can you elaborate? |
look carefully, it handles when
nope
|
note that the test never made it into master because it was "not useful" #8145 |
Well, I agree the test would've been useful 😉 I didn't catch that it indeed didn't end up being committed... Seems to me like this should really be handled correctly in the stdlib by |
feel free |
Contrary to what I assumed in python#9143, shutil.get_terminal_size() doesn't actually handle a 0 width from os.get_terminal_size() - it only handles a 0 COLUMNS environment variable. Thus, this caused python#8144 to regress. This change re-adds and uses DEFAULT_COLUMNS and also adds the test which was deemed unnecessary in python#8145 - this regression proves that I'd have been a good idea to add it in the first place. (Test written by Anthony Sottile) Fixes https://github.com/pre-commit/mirrors-mypy/issues/29
Sorry for the trouble - here's a fix: #9651 |
Contrary to what I assumed in #9143, shutil.get_terminal_size() doesn't actually handle a 0 width from os.get_terminal_size() - it only handles a 0 COLUMNS environment variable. Thus, this caused #8144 to regress. This change re-adds and uses DEFAULT_COLUMNS and also adds the test which was deemed unnecessary in #8145 - this regression proves that I'd have been a good idea to add it in the first place. (Test written by Anthony Sottile) Fixes https://github.com/pre-commit/mirrors-mypy/issues/29
This PR does three related cleanups (cc'ing @ilevkivskyi who seems to have implemented most of the underlying code):
Use
shutil.get_terminal_size()
The documentation for Python's
os.get_terminal_size()
says:shutil.get_terminal_size()
already takes care of various corner cases such as:DEFAULT_COLUMNS
)os.get_terminal_size()
raisesOSError
In addition to that:
os.get_terminal_size()
is unsupported")COLUMNS
environment variable (rather than the non-standardMYPY_FORCE_TERMINAL_WIDTH
)Move
MYPY_FORCE_TERMINAL_WIDTH
handling intoget_terminal_width()
Those two callers are the only two, and both check
MYPY_FORCE_TERMINAL_WIDTH
before callingget_terminal_width()
.Remove
fixed_terminal_width
fromutils.colorize
docstringThis was added by 91adf61, yet
colorize()
doesn't actually have afixed_terminal_width
argument.