Fixing Windows color regression #5327
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
| @@ -50,14 +49,6 @@ | |||
| format_json = False | |||
| invocation_id: Optional[str] = None | |||
|
|
|||
| # Colorama needs some help on windows because we're using logger.info | |||
There was a problem hiding this comment.
@nathaniel-may I know you added this in PR ##4474. I don't see a difference with it included or not in my testing. Can you think of scenario for me to test that would make code actually important here? It looks like we always call the logger.py that has the initialization we want, but I wanted to check with you first
There was a problem hiding this comment.
On Windows, calling init() will filter ANSI escape sequences out of any text sent to stdout or stderr, and replace them with equivalent Win32 calls.
from the colorama readme.
You could test by logging a message that includes an ansi escape sequence such as ESC[31m which should make the following characters have a red background. Without calling init on Windows with that message it might have odd behavior.
| logging_stdout = sys.stdout | ||
| if sys.platform == "win32": | ||
| if not os.getenv("TERM"): | ||
| logging_stdout = colorama.AnsiToWin32(sys.stdout).stream |
There was a problem hiding this comment.
colorama.AnsiToWin32(sys.stdout).stream is only used when wrap=False. When changing to hit this for Windows only terminals, we can set wrap=True and drop the AnsiToWin32 call
| @@ -455,7 +454,7 @@ def emit(self, record: logbook.LogRecord): | |||
|
|
|||
|
|
|||
| class LogManager(logbook.NestedSetup): | |||
| def __init__(self, stdout=logging_stdout, stderr=sys.stderr): | |||
There was a problem hiding this comment.
Removed logging_stdout above
* Fixing Windows color regression * Cleaning up logic * Consolidating logic to the logger * Cleaning up vars * Updating comment * Removing unused import * Fixing whitespace * Adding changelog (cherry picked from commit e48f7ab)
* Fixing Windows color regression * Cleaning up logic * Consolidating logic to the logger * Cleaning up vars * Updating comment * Removing unused import * Fixing whitespace * Adding changelog (cherry picked from commit e48f7ab) Co-authored-by: leahwicz <[email protected]>
resolves #5191
Description
With recent changes to Colorama logic in PR #4792, a regression was introduced that stopped logs from having color for Windows. This PR fixes that and reworks some logic that seems duplicated in 2 places but not really needed. The original fix was to keep color in logs when piped for MacOS and Linux which I verified still is happening with the changes
ℹ️ The original logic for Colorama in logger.py is 5 yrs old. I'm not sure why we did it the way we did it but I simplified to initialize Colorama only on Windows for non-Unix based terminals. Originally we tried to initialize it for all OS's but this led to the color not being preserved in the stdout that was addressed in #4792. My test matrix is below on all the combos I tried out and saw the same colored output.
This is what I tested and screenshots of my output:
Windows CMD:
Windows Git Bash:
Windows Git Bash with piping output:
Mac iTerm:
Mac iTerm with piping output:
Checklist
changie newto create a changelog entry