Added FORCE_COLOR and NO_COLOR environment variables

[jhol]

This patch add support for the FORCE_COLOR and NO_COLOR environment variables which are a de-facto standard for overriding automatic behaviour in circumstances where colors are required in redirected (non-TTY) output streams, or if colors are required to be removed in non-redirected (TTY) output streams.

[jhol]

By coincidence, this merge request also feeds into the wider NO_COLOR effort going on at https://no-color.org/

[eine]

Hi @jhol! Thanks for proposing this PR and for the reference to no-color.org. I'm coming from tox-dev/tox#1468, and I wonder if you'd consider checking PY_COLORS too. Should you consider that PY_COLORS is not general enough and that the issue should be handled elsewhere, I'd be ok with it. I'm just looking for other thoughts on the point.

[jhol]

@1138-4eb You're welcome to start a branch on top of mine and add an additional patch to add the functionality you're looking for.

It should be simple. I'd do it myself, but I don't have time to do the testing right now.

[jhol]

@tartley is this PR likely to be accepted?

[jhol]

@tartley - sorry to nag. Do you know if you will have time to look at this any time soon?

[tartley]

Hi, @jhol Thanks heaps for the PR. Apologies I hadn't noticed it previously. It sounds eminently reasonable. I'll take a look at it...

[tartley]

This PR is a great idea, and the code changes are great.

I have some thought though! (sorry!) See the comments in the diffs. Thank you!

Also! Changing the code also requires changing the tests.

[eine]

I was reminded recently that many people rely on Colorama, when I accidentally broke a release and was instantly deluged with issue reports, emails and tweets.

That sounds really humble, considering that there are ~72k dependents ☺️ .

So we'll have to think about this carefully. Either you can do it, and write about it to demonstrate that you definitely understand what's going on, or else I can have a think about it sometime and try to figure it out.

I can contribute a GitHub Actions (GHA) workflow that would test this in:

• Windows:
  • Cmd + Python native
  • Powershell + Python native
  • Git for Windows + Python native
  • MSYS2 + Python native
  • Cmd + Python from MSYS2
  • Powershell + Python from MSYS2
  • Git for Windows + Python from MSYS2
  • MSYS2 + Python from MSYS2
• GNU/Linux
• macOS

Unfortunately, GHA does neither provide nor fake a TTY (actions/runner#241). Hence, the GNU/Linux environment is the only one where we can get one (in a container). There are two options to test the effect of FORCE_COLOR, NO_COLOR and <> in this context:

• Add some mechanism to the codebase to override isatty for testing purposes.
• Not a tty actions/runner#241 (comment)

What do you think?

[tartley]

@eine That does sound fabulous, thank you for the thoughts.

The 'actions/runner' option sounds promising, but seems to be stalled on exactly how to implement it? Or am I misreading?

The 2nd option could at least partially lean on unit tests to exercise the details of this code even if no tty is actually present. (My memory of this code is vague, but I would hope that is what is currently present)

eg1. Pass fake streams that will look like they do or do not have a tty:

stream_with_tty = Mock(closed=False, isatty=lambda: True)

def test_xxx(self):
    ansiToWin32 = AnsiToWin32(stream_with_tty)

eg2. Or maybe inject a fake StreamWrapper class?

fake_sw = Mock(closed=False, isatty=lambda _: True)

@patch("colorama.ansitowin32.StreamWrapper", fake_sw)
def test_xxx(self)
    # inside this constructor call, "isatty" will evaluate to True
    ansiToWin32 = AnsiToWin32(...)

But, you are right, it would be nice to ALSO have some end to end test that sanity checks a couple of core scenarios with and without a tty present. I think when I first wrote this, I punted on that, and wrote "demo" scripts instead, which should produce relatively easy-to-check output for humans. 🤮

[eine]

@tartley, as a starting point, I created #249. It does not run any "mocked" check yet. Instead, the demos are executed. The next step is to write a proper test.

However, I based the PR on this one, so I added three tests to each job. I found that FORCE_COLOR and NO_COLOR seem to be ignored. See, for example, https://github.com/eine/colorama/actions/runs/33252844 @jhol, any guess?

[jhol]

@eine I reset the PR back to my original submission without @tartley 's suggestions, because they prevent the patches from working correctly. I don't know how we can resolve this, because as far as I can see... everything in my patch-set is necessary for it to work as intended:

Here is a series of tests:

• Running in a Linux terminal
• Running in a Linux terminal with NO_COLOR set
• Piping to a file on Linux
• Piping to a file on Linux with FORCE_COLOR set
• Running in a windows console window
• Running in a windows console window with NO_COLOR set.

In addition, I have tested:

• Running in a Msys2 terminal
• Running in a Msys2 terminal with NO_COLOR set
• Piping to a file on Msys2
• Piping to a file on Msys2 with FORCE_COLOR set

...and in each case the result is correct.

How can we resolve the gap between the comments received on this PR and the tests I have done that pass?

[eine]

@jhol I rebased my PR on top of your branch, and on top of master. I can confirm that it now works as expected for all the tested cases: https://github.com/eine/colorama/actions/runs/38098545

How can we resolve the gap between the comments received on this PR and the tests I have done that pass?

Ideally:

1. Rebase this PR on top of master.
2. Add programatic checks to Add GitHub Actions workflow 'test' #249 and merge it.
3. Rebase this PR on top of Add GitHub Actions workflow 'test' #249 and add corresponding programatic checks.
4. Apply @tartley's proposals one by one. Some of them will break the checks.
5. When all the tests are green, merge this PR.

Alternatively, we can skip adding programatic checks to #249, and/or merging it. You can still rebase on top of it to have all the tests executed automatically when you push. However, you need to visually check all the jobs.

[jhol]

@eine

1. Rebase this PR on top of master.

Done. Specifically rebased on top of your #249 patch.

2. Add programatic checks to Add GitHub Actions workflow 'test' #249 and merge it.

See above.

3. Rebase this PR on top of Add GitHub Actions workflow 'test' #249 and add corresponding programatic checks.

Would you be able to handle this? It's a bit beyond the scope of what I have time to get into right now.

4. Apply @tartley's proposals one by one. Some of them will break the checks.

The logic certainly was certainly getting rather confusing, so I used Yosys to optimize the logic:

It's a little better than it was before, but it's still a rather convoluted set of decisions - irreducibly so.

Despite what @tartley said, it won't work if I take out the changes to the strip decision.

5. When all the tests are green, merge this PR.

The results of the test runs and the test suite are both passing now.

[eine]

@tartley I resolved merge conflicts in #249. @jhol, you should be able to merge https://github.com/eine/colorama/tree/ci-gha here.

[jhol]

@eine Thanks for resolving the merge conflicts. I fast-forwarded this branch to include your merge-commit.

[jhol]

@eine @tartley - Ok this passes the build server now except for one of the macos builds: https://github.com/vcatechnology/colorama/runs/4225500386?check_suite_focus=true

[hugovk]

@eine @tartley - Ok this passes the build server now except for one of the macos builds: vcatechnology/colorama/runs/4225500386?check_suite_focus=true

Need to do something like #330 to fix PyPy3 on macOS.

[jhol]

@eine @tartley - Ok this passes the build server now except for one of the macos builds: vcatechnology/colorama/runs/4225500386?check_suite_focus=true

Need to do something like #330 to fix PyPy3 on macOS.

Yes - that looks like it would fix it. Hopefully we can get this merged soon.

[ethnh]

Any updates on this?

[ssbarnea]

With all due respect for colorama, but why do we need to commemorate getting this simple and logic PR into its 3rd year of its life?

This being said, I am not surprised rich newer versions removed colorama.

[tartley]

@ssbarnea It's a fair question. The answer is that I haven't used Windows for anything but gaming for years (a decade? Almost two?), so have had no actual motivation to add new features to Colorama, in combination with things like becoming a parent, which vastly reduces my available time for projects like this. @wiggin15 has been doing an amazing job on maintenance for years and years, but also suffers from natural human fatigue.

Also, Colorama has a very large install base, including critical Python ecosystem tools like pip, so it's important for us not to break any subtle behaviors that those users rely on. (in one infamous historical colorama incident, publishing a mere README tweak broke millions of people's ability to use Pip to install any other project that included colorama as a transitive dependency. We have to be careful.)

Those two factors combined means there is a very natural and substantial incentive to simply not merge new things. If anyone else wants to step up to be a maintainer, you'd be very welcome.

I was not aware of the Rich project's decision, but that makes perfect sense for them, and I endorse the decision.

[tartley]

@ssbarnea Also, while large amounts of the blame rest on me for not being responsive to PRs like this, also, this particular PR has had real problems through it's life. I've been very explicit in my comments above, that it needs automated tests, and it still has none, so should not be merged. If it had tests, I like to think it would have been merged by now.

Firing up 'demo.sh' for humans to eyeball the results, while nice, is not an appropriate replacement.

Merging this as-is, as with any untested code, simply isn't fair on future developers. It is punting the work that should be done in this PR onto other people. Years from now, someone else will be trying to merge some innocuous looking changes that break this code. This is inevitable. Nobody can write code perfectly that never breaks anything. But we are saved from merging our mistakes by the tests. Without tests to warn us that this code has been broken, other developers will have no guard rails to prevent them merging broken code. Then, when a broken release goes out, it's me, or some other innocent third party, who will be dealing with the tsunami of issues, emails and tweets from angry and desperate users.

[ssbarnea]

@tartley Thanks for these awesome words. Yep, I know that maintaining is 99% pain, I have a long list of things that mostly depend on me... and I know that there is a category of entitled users ready to troll the maintainer about things he did wrong or did not do. Those do a lot of damage to us.

While I do have a lot of experience with packaging and testing (GHA) on python packages, I am not sure if I should raise my hand for colorama, even if I used it for years, mostly because the amount of time I spend on Windows is likely 2%, the rest being Windows and MacOS.

What scares me the most is that I do not see any checks reporting on this pull-request... without active CI I would be really afraid to do anything.

[tartley]

Yep, fair. When I first wrote this code many years ago, I just expected devs to run the tests locally. Then we started using Travis, but after it imploded development has not been active enough to instate anything else.

[ssbarnea]

I still do not see GHA running on this PR and I only original OP or a maintainer can use the close/open trick to force the pipelines to run again.

There is also another aspect: colorama is one of the very few projects that did not remove support for obsolete versions of python. This prevents the project from moving forwards as most of the tools/libraries around dropped support for it. Most attempts to fix something will get stuck on problems like this. I would not be surprised to see python 2.7 removed from GHA soon too, so it will be impossible to even test it.

Someone needs to be able to make decisions in order to keep this project alive, or it will be killed by strangulation. Bump major version and drop anything that is outdated, allow others to contribute and give the project a new life. Keeping a stable/.. branch would be enough in order to allow potential emergency hotfixes for ancient versions.

I personally used to keep compatibility for very long time but in 2022 I managed to move most of the projects I maintain to require python 3.8+, and some of them have more github stars than colorama. It is bit fun as at beginning of 2021, I was still having projects supporting py27 on the main branch,... so dropped a big number of python version in a short period of time. I mentioned this because the boost in project ease of maintain was immense. The days of hoarding EOL python versions are gone. If someone really wants to backport a fix, they have to do the leg-work. Based on my experience, almost nobody did the work, they we motivated to spent the time upgrading their systems instead.

As long project uses semantic versioning, people understand what a major version mean. Those that installed dependencies without any kind of version ranges are supposed to read the results of their own mistakes.

[jhol]

Also, while large amounts of the blame rest on me for not being responsive to PRs like this, also, this particular PR has had real problems through it's life. I've been very explicit in my comments above, that it needs automated tests, and it still has none, so should not be merged. If it had tests, I like to think it would have been merged by now.

I'm happy to write a test - I just don't really know how to go about it.

[kasperk81]

https://no-color.org/

but FORCE_COLOR has no such effort. the closest thing to documentation is
https://nodejs.org/api/cli.html#force_color1-2-3
and it is not just "setting this variable to any value" as NO_COLOR spec describes, rather its values are more involved: 1 => 16 color, 2 => 256 color, 3 => 16 million colors. any other value disables the color if it was enabled (which is unnecessary when in the same documentation they are also supporting NO_COLOR ...). node.js is a good example of how to over engineer stuff.

[tartley]

Needs automated tests

[tartley]

I don't think we can merge this. It mingles different kinds of changes and includes no tests. The investigation and thought has been valuable, but it has also exposed that this change is complicated. Because of that, I'm going to close this PR as too risky. If someone wants to resurrect the idea in a new MP, with tests, without blending in other changes like the GHA, then I'd be receptive. Let's talk about it before you do the work though.