Check for 'closed' attribute on 'wrapped' in AnsiToWin32 #84
Conversation
4e0840c to
8136165
Compare
8136165 to
b29cb92
Compare
| self.strip = strip | ||
|
|
||
| # should we should convert ANSI sequences into win32 calls? | ||
| if convert is None: | ||
| convert = conversion_supported and not wrapped.closed and is_a_tty(wrapped) | ||
| convert = (conversion_supported and |
There was a problem hiding this comment.
Not sure if and here is right, compared to the or above?!
There was a problem hiding this comment.
The and and or are ok.
- We want to strip the ANSI codes in two separate cases: either we're using a Windows cmd-like terminal (conversion_supported, ANSI codes are not supported) OR we're not using a tty at all (e.g. output redirection)
- To convert the ANSI codes to Windows API we require two conditions: use a cmd-like terminal AND use a tty (no redirection).
|
This looks good to me, but I fear that the two lines for 'convert' and 'strip' are becoming too long and compilcated (I also prefer not to have a line break after |
Thought about that initially, but then it was clearer that way somehow (inline). I've wondered even if for such a stream it should not be true (given that it's something not connected to a term)?! |
| self.strip = strip | ||
|
|
||
| # should we should convert ANSI sequences into win32 calls? | ||
| if convert is None: | ||
| convert = conversion_supported and not wrapped.closed and is_a_tty(wrapped) | ||
| convert = (conversion_supported and | ||
| hasattr(wrapped, 'closed') and not |
There was a problem hiding this comment.
Personally, I'd put the 'not' on the next line - it does apply to the 'wrapped.closed'. Ditto the 'not' at the end of line 68.
|
I'd be OK with everything, and also if you cherry-pick and amend it. Please note that this caused and still causes a lot of bug reports for jedi-vim, which uses colorama internally, and therefore it would be nice to get this fixed soon. |
|
Hi @blueyed . I created a similar patch here: |
|
Given the test it should work. |
|
This bug is in the version of colorama that is currently due to be shipped in Ubuntu 16.04. It'd be great if a new release could be cut so we didn't have to maintain a diff from upstream for the next 5 years. :) |
|
(The bug tracking this problem in Ubuntu is https://bugs.launchpad.net/ubuntu/+source/python-colorama/+bug/1554129, for reference) |
|
Done. Thanks. |
|
This has been uploaded to Debian and sync'd to Ubuntu, so it will be in 16.04. Thanks! |
Otherwise colorama might cause crashes when used in Vim (davidhalter/jedi-vim#522 (comment)) or Neovim (davidhalter/jedi-vim#522 (comment)).