Prevent file stream from being incorrectly wrapped on Pycharm

[Delgan]

Hi.

This is related to a comment I left some days ago: #163 (comment)

I think this is quite important to only wrap stdout and stderr on Pycharm, rather than any stream. Think for example of a logging module which relies on colorama to wrap arbitrary handler's stream. 😉

[wiggin15]

colorama.init only wraps stdout and stderr. Out of curiosity, what is your use case for wrap_stream for other streams, and do those streams support coloring?

[wiggin15]

Do we want to check against the __ versions of stdout and stderr instead of using sys.stdout and sys.stderr? If there was another redirection (so the values of the "regular" sys streams changed), we would still want to return "True" under PyCharm for these streams, no? Since init uses sys.stdout and sys.stderr directly, it makes sense to check against these streams. Perhaps I'm missing something though.

[Delgan]

My reasoning was the following: the stdout and stderr under Pycharm are inherently bugged. These two streams are unambiguously accessible through __stdout__ and __stderr__. If the user happens to override sys.stdout, then we do not have much information about the stream that will replace it (it could be a file or a StringIO), so we should not assume that the replacing stream is bugged too, instead we should check as usual using stream.isatty().

Basically, the edge case is an user replacing sys.stdout with a non-tty stream on Pycharm. In such a case, colorama.is_a_tty(sys.stdout) should return False, but this would not be the case if the check is made using stream is sys.stdout.

Does it make sense to you? I do not exclude having missed something too.

[wiggin15]

I think I understand (a user custom sys.stdout may not be the real "PyCharm terminal" and so may need to be treated as "not a tty"). The problem is that after "colorama.init", sys.stdout will be replaced by colorama and then colorama.is_a_tty(sys.stdout) will return False (in PyCharm) although it is a simple wrapper of the "original". Maybe we should check for both? i.e. something like
... and (stream in (sys.__stdout__, sys.__stderr__, sys.stdout, sys.stderr))?

If I understand correctly, the problem is generally only when using the PyCharm interactive console, and shouldn't affect scripts running outside of the IDE.

[Delgan]

Oh, you are right, I did not even think of it, thanks for pointing this out...

But I do not see how checking for stream in ... will prevent the edge case of user replacing sys.stderr with a non-tty stream (on Pycharm). 😕

In my opinion, as we are wrapping a broken sys.__stderr__ stream, we should take the opportunity to fix it. Something like this:

class StreamWrapper(object):
    ...

    def isatty(self):
        return is_a_tty(self.__wrapped)

It seems to fix all the edge cases we discussed, what do you think?

[Delgan]

Thanks for your answer!

I am not much worried about init(), I know it will continue to work as expected. I am rather concerned about colorama.is_a_tty() and AnsiToWin32().

My use case is actually a logging function which accept any stream object for the handler (and later call stream.write(message)), so it could be passed indiferently open("file.log", "w") or sys.stderr. If the stream is a "tty", I would like to being able to colorize it using colorama.

I am planning to do something like this:

if colorama.is_a_tty(stream):  
    stream = AnsiToWin23(stream).stream

But I think I will be facing an edge case on Pycharm.

[Delgan]

Hey @wiggin15!

I just updated my PR and added a bunch of unit tests to check colorama.is_a_tty() against the different edge cases we discussed.

Quick summary, assuming a Pycharm environment with broken stdout (and stderr):

x	x.isatty()	is_a_tty(x)	Comment
sys.__stdout__	False	True	This object is used as a reference to know whether or not is_a_tty() is called on the original broken Pycharm stream to be fixed
sys.stdout	False	True	Assuming sys.stdout left to its original broken value, others tests should consider that user can override this at runtime with tty or non-tty streams
StreamNonTTY()	False	False	Would return True without this PR, also your ... and (stream in (sys.__stdout__, sys.__stderr__, sys.stdout, sys.stderr)) solution would not work here if user overriden sys.stdout with StreamNonTTY()
None	Error	False	If sys.__stdout__ is None (which may happen in rare cases according to the docs), is_a_tty(None) should return False hence the stream is not None check
StreamWrapper( sys.stdout)	True	True	PR update fixes this, otherwise it would have returned False as sys.stdout is broken
StreamWrapper( StreamNonTTY())	False	False	Not original streams should not be fixed

[wiggin15]

This looks ok, I guess. If I understand correctly, this shouldn't have an effect outside of PyCharm IDE, right?

[wiggin15]

This is interesting. Maybe we don't even need the functions is_a_tty and is_stream_closed above, and they should instead be methods of StreamWrapper, so that calling these methods will return what colorama "thinks" instead of the original. i.e. put the code here instead, and call the methods instead of the functions later in the code. What do you think?

[Delgan]

This looks ok, I guess. If I understand correctly, this shouldn't have an effect outside of PyCharm IDE, right?

Absolutely, apart from Pycharm the behavior remains the same (a simple call to the isatty() function of the wrapped stream).

This is interesting. Maybe we don't even need the functions is_a_tty and is_stream_closed above, and they should instead be methods of StreamWrapper, so that calling these methods will return what colorama "thinks" instead of the original. i.e. put the code here instead, and call the methods instead of the functions later in the code. What do you think?

I think this would be much more elegant indeed! I do not see why this would be a problem, I implemented your suggestion.

[wiggin15]

Thanks!