Repair colorama wrapping on non-Windows platforms #1670
Conversation
|
Now looking at the colorama code, none of this "let's fake the detach method" is even required in the first place as
|
|
Saw that colorama's Perhaps this PR could go in as-is and then someone with a Windows environment could confirm that ripping-out the |
Perfect, I have a Windows machine right in front of me :-) I'll test the other idea now. |
|
Never faking the Regardless, it would be nice if you could update the nearby comments as some will become outdated with your patch or are already incorrect (like Oh, and if you'd like, feel free to add yourself to the authors list in the README! |
|
Sorry for forgetting about this but could you please add an entry in the change log ( Requesting review from @zsol as he reviewed the original PR that introduced coloured diffs... and the bug you're fixing. |
The wrap_stream_for_windows() function calls colorama.initialise.wrap_stream() function to apply colorama's magic to wrapper to the output stream. Except this wrapper is only applied on Windows platforms that need it, otherwise the original stream is returned as-is. The colorama wrapped stream lacks a detach() method, so a no-op lambda was being assigned to the wrapped stream. The problem is that the no-op lambda was being assigned unconditionally whether or not colorama actually returns a wrapped stream, thus replacing the original TextIOWrapper's detach() method. Replacing the detach() method with a no-op lambda is the root cause of the problem observed in psf#1664. The solution is to only assign the no-op detach() method if the stream lacks its own detach() method. Repairs psf#1664
- Clarify doc string and comments - Do not assign a no-op detach() method to AnsiToWin32 wrapper--that wrapper will pass the call through to the wrapped stream's detach() method. - Improve the control flow. - Correct type annotation for colorama.AnsiToWin32
There was a problem hiding this comment.
LGTM. Thanks for the contribution @jpgrayson and the thorough review @ichard26
* Repair colorama wrapping on non-Windows platforms The wrap_stream_for_windows() function calls colorama.initialise.wrap_stream() function to apply colorama's magic to wrapper to the output stream. Except this wrapper is only applied on Windows platforms that need it, otherwise the original stream is returned as-is. The colorama wrapped stream lacks a detach() method, so a no-op lambda was being assigned to the wrapped stream. The problem is that the no-op lambda was being assigned unconditionally whether or not colorama actually returns a wrapped stream, thus replacing the original TextIOWrapper's detach() method. Replacing the detach() method with a no-op lambda is the root cause of the problem observed in psf#1664. The solution is to only assign the no-op detach() method if the stream lacks its own detach() method. Repairs psf#1664
The
wrap_stream_for_windows()function callscolorama.initialise.wrap_stream()function to apply colorama's magic towrapper to the output stream. Except this wrapper is only applied on
Windows platforms that need it, otherwise the original stream is
returned as-is.
The colorama wrapped stream lacks a
detach()method, so a no-op lambdawas being assigned to the wrapped stream.
The problem is that the no-op lambda was being assigned unconditionally
whether or not colorama actually returns a wrapped stream, thus
replacing the original
TextIOWrapper'sdetach()method. Replacing thedetach()method with a no-op lambda is the root cause of the problemobserved in #1664.
The solution is to only assign the no-op
detach()method if the streamlacks its own
detach()method.Repairs #1664