-
Notifications
You must be signed in to change notification settings - Fork 73
Default ANSI color codes off when stdout is NOT a TTY #478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
oops, left some extra, unnecessary indentation in. Fixing... |
1778bb9 to
b4d1ff8
Compare
|
Oh cool, thanks |
common/log.cc
Outdated
| #elif defined(__unix__) | ||
| return isatty(STDOUT_FILENO) != 0; | ||
| #else | ||
| return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would false be a better default? Or should we even care about non-Windows platforms w/o POSIX support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we support any platforms in this category (non-Windows, non-POSIX) so probably doesn't matter either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case it could be simplified to #ifdef _WIN32 / #else with _WIN32 not being defined implying __unix__ is defined. It would be less misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure that works.
I checked the codebase and it's a bit inconsistent, but we do already assume lack of _WIN32 implies POSIX (in cmdlib.cc:q_aligned_malloc).
There's also a LINUX preprocessor macro we define ourselves in CMakeLists.txt if CMake reports a unix-like target; LINUX is used in some include guards but nothing else.
fb3c77c to
f50ae35
Compare
|
Nice, thanks.
so, all good. Is it OK if I revert the reformatting of |
|
Yeah that's cool, it's your repo after all :) Ah I see you made the change already |
|
Alright, that's enough updates from me unless you need me to squash the commits |
|
Thanks again. I'll squash it in GitHub. |
Prevents ANSI color codes from being printed in output in e.g. TrenchBroom without the need for -nocolor switch. Also prevents color codes from appearing when redirecting output, e.g.
qbsp my.map | catTested w/ TrenchBroom under Linux: No ANSI codes are printed
Tested w/ terminal under Linux (
qbsp some.map): Output displayed w/ colorTested w/ terminal under Linux/Wine (
wine qbsp.exe some.map): ANSI color codes are printedTested w/ terminal under Linux/Wine w/ redirected output (
wine qbsp.exe some.map | cat): No ANSI color codes are printed