search

goatshriek / stumpless

a C logging library built for performance and features
https://goatshriek.github.io/stumpless
Apache License 2.0
446 stars 335 forks source link

Terminal ansi colors #427

Closed DR-Reg closed 3 months ago

DR-Reg commented 4 months ago

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 commented 4 months ago

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

codecov[bot] commented 4 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

goatshriek commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.