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

Enable color logging on Windows via colorama #1817

Merged
merged 6 commits into from
Mar 26, 2017

Conversation

mivade
Copy link
Contributor

@mivade mivade commented Aug 31, 2016

This is a slight rework of #1487 which was closed when I reorganized my clone. I'm not sure what the proper way to test this would be, but I used the following script to visually check that the output does as expected:

import logging
from tornado.log import enable_pretty_logging

logger = logging.getLogger()
enable_pretty_logging()
logger.setLevel(logging.DEBUG)

logger.info("INFO")
logger.warning("WARNING")
logger.debug("DEBUG")
logger.error("ERROR")
print("normal")

This also uses dimmer colors for Windows. This can be tested with
the following script:

```python
import logging
from tornado.log import enable_pretty_logging

logger = logging.getLogger()
enable_pretty_logging()
logger.setLevel(logging.DEBUG)

logger.info("INFO")
logger.warning("WARNING")
logger.debug("DEBUG")
logger.error("ERROR")
print("normal")
```
tornado/log.py Outdated
@@ -126,6 +135,10 @@ def __init__(self, color=True, fmt=DEFAULT_FORMAT,
for levelno, code in colors.items():
self._colors[levelno] = unicode_type(curses.tparm(fg_color, code), "ascii")
self._normal = unicode_type(curses.tigetstr("sgr0"), "ascii")
elif colorama is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to be conditioned on the color variable and _stderr_supports_color(). Make this

    if color and _stderr_supports_color():
        if curses is not None:
           ...
        elif colorama is not None:
           ..
        else:
            raise RuntimeError("no supported color terminal library")

@mivade
Copy link
Contributor Author

mivade commented Feb 22, 2017

Any other comments, @bdarnell?

Just pinging re: your recent post on the mailing list.

tornado/log.py Outdated
import sys

from tornado.escape import _unicode
from tornado.util import unicode_type, basestring_type

try:
import colorama
colorama.init()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really comfortable calling colorama.init at import time. It looks like it may not be idempotent, and replacing sys.std{out,err} is intrusive enough that there should be a more definite opt-in than "the colorama package is installed" (colorama's issue tracker shows that its approach has quite a few quirks).

What if we leave the call to colorama.init() to the application and replace the if colorama is not None calls below with if sys.stderr is getattr(colorama, 'wrapped_stderr', object())? That way we'd be able to detect the use of colorama as an alternative indicator that ANSI color is supported (instead of curses's inspection of $TERM and the terminfo database)

It looks like windows 10 also has a native ANSI-aware terminal (see tartley/colorama#104), so another approach we could take would be to ignore colorama and figure out how to detect that terminal in the absence of curses and terminfo.

@mivade
Copy link
Contributor Author

mivade commented Feb 26, 2017

I agree with your objection to calling colorama.init explicitly. I've removed that and added language to the LogFormatter docstring pointing out that colorama must be initialized by the user.

I'm reluctant to rely on Windows 10 ANSI-aware terminal support for now since there is still a lot of use of Windows 7.

@bdarnell
Copy link
Member

Looking good, just one more thing (I think) to fix the build. Sphinx is complaining that it can't find colorama.init (single backticks are links in RST format, not just monospace like in markdown). Those need to be double backticks instead.

@mivade
Copy link
Contributor Author

mivade commented Mar 25, 2017

Thanks for the input. I was using the :func: directive there, but with colorama not installed, that resulted in the error. I've replaced with double backticks instead.

@bdarnell bdarnell merged commit 62d1c62 into tornadoweb:master Mar 26, 2017
@bdarnell
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants