Terminal ansi colors

[DR-Reg]

Adds support for using ansi escape codes to represent different severity levels when printing to a terminal, and presents some default escape codes. Allows for . Solves #103.

Unit tests were not written however, as I was unsure how to capture the stdout to check it was correctly printing ansi terminals (I did check with a script on my side that it was working).

This is my first open source contribution so any feedback is welcome.

[DR-Reg]

Do I need to 'sign the commit' so it can be merged? Or is anything else missing?

[codecov]

Codecov Report

Attention: Patch coverage is 96.29630% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.85%. Comparing base (ed85c54) to head (20553d0).
Report is 2 commits behind head on latest.

Files	Patch %	Lines
src/target/stream.c	96.22%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           latest     #427      +/-   ##
==========================================
+ Coverage   90.79%   90.85%   +0.05%     
==========================================
  Files          48       47       -1     
  Lines        4335     4384      +49     
  Branches      577      586       +9     
==========================================
+ Hits         3936     3983      +47     
- Misses        268      269       +1     
- Partials      131      132       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[goatshriek]

Sorry for the delay in response, I have been focused on a different project for a bit.

First of all, thank you for tackling this issue! As I am sure you saw in the issue history it has drawn many contenders but remains yet unconquered.

Signing commits is not a requirement for PRs here, particularly since they are typically squashed anyway. However, I do recommend you work through that process for your own education (if education is something you're into, at least). But don't worry about signing and re-pushing your work or anything like that.

At first glance this seems like a solid contribution that shouldn't need much re-shaping, but I need to set aside time to sit down and review it in detail. I will get back to you within the next week with a review.

[goatshriek]

Thanks again for putting this together, and your patience in waiting for a review. This is great work! The changes below should be relatively straightforward.

However, you will also need to write some tests for this functionality. This will be a little more involved - the way I would approach this is to create a file, and use the stream to create a new stream target that writes into it. You can then check the file contents after the writing to make sure that the expected escape codes are there (or not, depending on the test).

[goatshriek]

You're closing in on done! Just two more changes needed - you'll need to resolve the Windows test failures, which will probably require some refactoring of the tests themselves to be a bit more portable. You may need to create a new file instead of using the stderr/stdout targets, but I'll leave this up to you.

Also, please add a test where you attempt to call stumpless_set_severity_color with an incompatible target type, to make sure that this fails as expected.

[DR-Reg]

I have added some preprocessor directives so the syscalls to dup etc. are conformant with the required windows ISO C rules. In the end I thought it was better to redirect stdout/stderr since in theory (as I understand) files should not have these escape codes (unless set manually).

I am aware that it may be possible to make this code simpler using some sort of macro to distinguish between code needed for windows vs unix. (however I wasn't sure what your policy on macros is, I can rewrite the code in this way if you want).

Also if you think the file approach is cleaner I will rewrite the test.

Also note that for some reason running on my windows machine I get that the regex expression is invalid? Is this a bug in the windows version of gtest or my fault?

Sorry for the inconvenience.

Edit: PS. Should the test for stumpless_set_severity_color be in test/function/target/stream.cpp or elsewhere?

[goatshriek]

The tests have a more relaxed standard for macros and wrapping up OS-specific functionality, so I think what you've done here is fine, no need to rewrite at this point. As long as they run on the major tested systems (which are currently the ones covered by CI) then I think we're good to go.

Hmm, I'm not sure why the Windows regexes are not working properly - perhaps the underlying implementation is more strict about regular expressions with non-displayable characters in them? Does changing the escape character to \x1b instead of the octal value help?

Yes, I think that test/function/target/stream.cpp is the best place for the set severity color tests.

[DR-Reg]

In the end the regex bug was due to gtest having limited regex support on windows (as seen here). I assume they will add better support in the future, so I put in a bit of a 'janky' solution for windows using one of GTEST's processor macros (so when they do add the functionality the actual regex is called).
Is this ok?

[goatshriek]

This will be okay, though I will ask that you please add a comment to the test code before the preprocessor conditional that explains why this is done so that others reading the tests will know why this is being done without finding this conversation.

It looks like you just need to update the src/windows/stumpless.def file with the new function, and address some memory leaks that valgrind has identified, and you'll be closing in on done.

[goatshriek]

Thank you again for working through this, and I hope that you feel a sense of accomplishment for completing an issue that gave so many others quite a bit of trouble!

I try to give contributors a shoutout on X when I can, especially for larger contributions like this one. I don't see one listed on your profile here though: if you have an account let me know (via email or private Gitter if you want it to remain hidden) and I will post something there.