Skip to content
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

LogFormatter: allow color support when sys.stderr.isatty() == False #2647

Open
javabrett opened this issue Apr 24, 2019 · 1 comment
Open
Labels

Comments

@javabrett
Copy link
Contributor

LogFormatter:

class LogFormatter(logging.Formatter):

... has very nice support for color logging via curses (or colorama).

My issue is with the test sys.stderr.isatty() in:

tornado/tornado/log.py

Lines 55 to 71 in c447875

def _stderr_supports_color() -> bool:
try:
if hasattr(sys.stderr, "isatty") and sys.stderr.isatty():
if curses:
curses.setupterm()
if curses.tigetnum("colors") > 0:
return True
elif colorama:
if sys.stderr is getattr(
colorama.initialise, "wrapped_stderr", object()
):
return True
except Exception:
# Very broad exception handling because it's always better to
# fall back to non-colored logs than to break at startup.
pass
return False

The LogFormatter might be able to be attached to a handler which is not writing to stderr type stream, but to some other TTY-capable device, which I have in my case. The syserr TTY test prevents attaching the LogFormatter to such logger handler and having color support.

If I monkey-patch _stderr_supports_color() to lambda: True, and make sure I make a call to curses.setupterm(fd=...), passing the fd of my TTY-capable stream, then I can log with the color support from LogFormatter, which is nice.

Maybe the best fix is to change color: True to None, but default based on the detection above, or if set to True explicitly by the caller, honour that.

@bdarnell
Copy link
Member

Yeah, making the color argument three-valued (with None meaning "auto") seems like the way to go. That'll require a little refactoring to split the detection part of _stderr_supports_color from the initialization part, but that seems like a good idea anyway.

Now that I'm rereading the curses docs, it looks like we should also be passing stderr's file descriptor to curses.setupterm (otherwise it may send terminal escape characters to the wrong stream if stderr and stdout have different destinations).

@bdarnell bdarnell added the log label Apr 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants