Atexit fix

[3tilley]

A library we use throws this annoying error when it's called in under certain conditions. It's probably doing something wrong, but I think colorama should be more resilient to this particular bad state. I've attached the script that demonstrates the error. I've tried to write a test for this, but the best I can to do honestly test the functionality is to spin up a new Python in a subprocess that calls colorama, this seems awkward and was difficult to make portable. If it's your desire I'll do it though.

from __future__ import print_function

import codecs
import sys

from colorama import init

init()
sys.stdout = codecs.getwriter("utf-8")(sys.stdout.detach())

print("foo", sys.stdout)

Before and after the fix:

Rejected test attempt:

    def testDetachAfterInitDoesntThrow(self):
        fd, name = tempfile.mkstemp(suffix=".py", text=True)
        sample_python = """
from __future__ import print_function

import codecs
import sys

from colorama import init

init()
sys.stdout = codecs.getwriter("utf-8")(sys.stdout.detach())

print("Hello", sys.stdout)
        """
        temp_script = Path(name)
        with temp_script.open(mode="w") as f:
            f.write(sample_python)
        output = subprocess.check_output(["python", str(temp_script)], universal_newlines=True)
        print(output)
        assert "buffer" not in output

[3tilley]

@tartley do you might approving me?

[tartley]

Hi there! Thanks for the contribution, this is going to be a great fix.

I can't approve until we have a working test of some sort though. My motivation is that future contributors need some automated way for them to be notified if they make a change which completely breaks your change, so that we don't then deploy their broken change because we don't know about it.

How about something like the following? The first test demonstrates the existing AttributeError handling, and the second one demonstrates the ValueError that you are adding:

class StreamWrapperTest(TestCase):

    ...

    def test_closed_shouldnt_raise_on_closed_stream(self):
        stream = StringIO()
        stream.close()
        wrapper = StreamWrapper(stream, None)
        self.assertEqual(wrapper.closed, True)

    def test_closed_shouldnt_raise_on_detached_stream(self):
        stream = TextIOWrapper(StringIO())
        stream.detach()
        wrapper = StreamWrapper(stream, None)
        self.assertEqual(wrapper.closed, True)

As always, you can tell whether these tests are actually working using TDD, or simulating a bit artifically in this case, since we already have the code changed:

1. remove the try...finally from StreamWrapper.closed
2. add the tests and run them, they should both fail
3. reintroduce the try...finally, only handling AttributeError
4. rerun the tests, the first should pass
5. make your change, adding the ValueError
6. rerun the tests, they should both pass

[3tilley]

Great idea, added. I tried something similar before but wasn't able to detach a StringIO - I clearly don't have your deep stream knowledge!

[3tilley]

I think this is just waiting a CI run, @tartley do you mind kicking it off?

[3tilley]

Thanks so much! I literally looked at this in an output today and thought back to this PR