main: fix replacing color codes in tracebacks

[FFY00]

Signed-off-by: Filipe Laíns [email protected]

[gaborbernat]

Can we add a test to validate the change?

[FFY00]

I think adding a test for this that would actually be helpful in development would be very tricky, and I feel like it might not be worth the time. The code that we would be covering would only generate formatting errors, not functional ones which would prevent people from working.
How do you test this kind of stuff in tox?

[gaborbernat]

Why would it be tricky? If now the contract is that the output is nicely colored, why would a test validating that be not worth it? tox threats output similarly. We run the code in a subprocess, and then assert the stdout/stderr contains the right series of bytes. If you can run it without needing a new subprocess you can assert on capsys/capfd and do the same. Doesn't have to test color output for all functional cases, just one run that covers all cases. Bugs related to the coloring helper function can be tested by directly calling the helper function from your test.

[gaborbernat]

In general looks good, let's just add an environmetn variable to enable color mode 😊

[gaborbernat]

I like that we have an environment variable to disable color, however can we have also one that force enables it? (e.g. tox 4 cannot forward the tty state into a subprocess yet, so is handy in such cases to force color mode). Perhaps FORCE_COLOR? And if neither set then check the tty state 😊

[FFY00]

Sure. What happens with both FORCE_COLOR and NO_COLOR?

[gaborbernat]

In my implementation NO_COLOR wins, but 🤷

[gaborbernat]

See e.g. how this would be used https://github.com/tox-dev/tox/blob/rewrite/tox.ini#L79 (this shows how mypy handles it).

[FFY00]

Done.

[gaborbernat]

Shouldn't empty values translate as false for these flags?

[FFY00]

https://no-color.org/ says the mere presence enables the behavior, the value is irrelevant. I think we should have the same behavior in FORCE_COLOR.

[henryiii]

I'm a fan of having a FORCE_COLOR too, as often on GHA (which supports color) things can be missing color. I think no-color.org went overboard - it's a pretty well established practice for an empty environment variable to be the same as unsetting it. It's not always easy to unset an environment variable, and the disconnect there is irritating. But I'd follow what the spec dictates. :(

[FFY00]

I understand the concern and share a similar opinion. IMO, coloring should be disabled if NO_COLOR is present and is not no/false. But given that it is already a pretty well established standard, I think the best option is to follow what is defined.

[layday]

It's not entirely clear what this is fixing. Are you trying to avoid replacing "[dim]" and "[reset]" in the traceback itself?

[FFY00]

Yes, as @henryiii pointed out.

[gaborbernat]

[FFY00]

The tests are failing on Windows and I don't really get why. Since I don't have any Windows setup right now, it might be a little while before I am able to set one up and debug the issues. If anyone is able to look into it, I would really appreciate it.
@henryiii you use Windows, right? Perhaps you could help out if you have time.

[FFY00]

It was colorama. It's a bit weird since this test was passing previously.

[henryiii]

The tests are failing on Windows and I don't really get why. Since I don't have any Windows setup right now, it might be a little while before I am able to set one up and debug the issues. If anyone is able to look into it, I would really appreciate it.
@henryiii you use Windows, right? Perhaps you could help out if you have time.

Sorry, wasn't around a computer much this weekend. I technically have a Windows box I boot up once in a while when I'm stuck debugging Windows issues in various packages. I'm perfectly fine to keep that a secret, though. 😅