Clean up terminal width handling#9143
Merged
JukkaL merged 3 commits intopython:masterfrom Jul 22, 2020
The-Compiler:terminal-width
Merged
Clean up terminal width handling#9143JukkaL merged 3 commits intopython:masterfrom The-Compiler:terminal-width
JukkaL merged 3 commits intopython:masterfrom
The-Compiler:terminal-width
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
Contributor
|
this regressed 9dcccca |
Contributor
Author
|
@asottile Can you elaborate? |
Contributor
look carefully, it handles when
nope |
Contributor
|
note that the test never made it into master because it was "not useful" #8145 |
Contributor
Author
|
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 |
Contributor
|
feel free |
Contributor
Author
|
Sorry for the trouble - here's a fix: #9651 |
JukkaL
pushed a commit
that referenced
this pull request
Oct 28, 2020
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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()raisesOSErrorIn addition to that:
os.get_terminal_size()is unsupported")COLUMNSenvironment variable (rather than the non-standardMYPY_FORCE_TERMINAL_WIDTH)Move
MYPY_FORCE_TERMINAL_WIDTHhandling intoget_terminal_width()Those two callers are the only two, and both check
MYPY_FORCE_TERMINAL_WIDTHbefore callingget_terminal_width().Remove
fixed_terminal_widthfromutils.colorizedocstringThis was added by 91adf61, yet
colorize()doesn't actually have afixed_terminal_widthargument.