-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Colorize live-log levelnames #3142
Conversation
c7d729c
to
216b21a
Compare
87013c4
to
d797e3f
Compare
d797e3f
to
ed1051f
Compare
What do you think about this? Are there tests that test the text colors of the pytest output, where I can take a look at? |
Unfortunately no AFAIK. 😕 |
Note that I successfully tested this PR in cpython2.7 and in cpython3.6. |
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.
This looks great @Thisch, please take a look at my comments! 👍
_pytest/logging.py
Outdated
DEFAULT_LOG_FORMAT = '%(filename)-25s %(lineno)4d %(levelname)-8s %(message)s' | ||
DEFAULT_LOG_DATE_FORMAT = '%H:%M:%S' | ||
|
||
# move to __init__ of ColoredLevelFormatter? | ||
LEVELNAME_FMT_REGEX = re.compile(r'%\(levelname\)([+-]?\d*s)') |
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.
Probably both LEVELNAME_FMT_REGEX
and LOGLEVEL_COLOROPTS
would be better off being class attributes of ColoredLevelFormatter
.
_pytest/logging.py
Outdated
def __init__(self, *args, **kwargs): | ||
super(ColoredLevelFormatter, self).__init__( | ||
*args, **kwargs) | ||
if py.std.sys.version_info[0] >= 3: |
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.
Please use six.PY3
instead (this will make it easier later to find python2-specific code)
_pytest/logging.py
Outdated
fmt = self._level_to_fmt_mapping.get( | ||
record.levelno, self._original_fmt) | ||
|
||
if py.std.sys.version_info[0] >= 3: |
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.
Ditto here for six.PY3
👍
_pytest/logging.py
Outdated
LEVELNAME_FMT_REGEX = re.compile(r'%\(levelname\)([+-]?\d*s)') | ||
|
||
|
||
class ColoredLevelFormatter(logging.Formatter): |
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.
To test this, probably the way to go is to unittest the ColoredLevelFormatter
class directly: create an instance and pass different LogRecord
instances and ensure they have the correct markup applied. Also interesting to use custom log levels and ensure they don't have any markup.
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.
Unfortunately this does not work, since tw.markup()
does not add ANSI escape sequences around the string in the unit tests:
(Pdb++) tw.markup('abc', red=True)
'abc'
(Pdb++) tw.hasmarkup
False
Any ideas?
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'll do some mocking and then it should work.
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.
ping @nicoddemus
ed1051f
to
c1da614
Compare
I guess that this is already too late for pytest 3.4, right? |
BTW, I don't understand why the tests fail. |
I plan to release 3.4 when I get home later, but if we merge this until then I will be happy to generate the package again.
No idea either, seems to be some bad interaction with |
c1da614
to
6c0a8ac
Compare
Let's hope that the tests now pass. |
Just remembered this now and would like to confirm, passing |
6c0a8ac
to
1ad52b9
Compare
Now the color config option should be taken into account. This made the monkeypatching code obsolete. |
_pytest/logging.py
Outdated
@@ -376,7 +432,10 @@ def _setup_cli_logging(self): | |||
log_cli_handler = _LiveLoggingStreamHandler(terminal_reporter, capture_manager) | |||
log_cli_format = get_option_ini(self._config, 'log_cli_format', 'log_format') | |||
log_cli_date_format = get_option_ini(self._config, 'log_cli_date_format', 'log_date_format') | |||
log_cli_formatter = logging.Formatter(log_cli_format, datefmt=log_cli_date_format) | |||
if ColoredLevelFormatter.LEVELNAME_FMT_REGEX.search(log_cli_format): | |||
log_cli_formatter = ColoredLevelFormatter(self._config, log_cli_format, datefmt=log_cli_date_format) |
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 could also perform the color check in the condition in line 435. Then I don't need to pass self._config
to the __init__
method of ColoredLevelFormatter
.
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.
Seems better
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 didn't know that color is a ternary type ('yes', 'no' and 'auto'). So I can't add self._config.option.color == 'yes' and
to the if condition. If the value of the color option is 'auto' the TerminalWriter.init determines if the terminal supports markup or not.
An alternative would be to pass a terminalwriter object (_pytest.config.create_terminal_writer(self._config)
) as the first arg of ColoredLevelFormatter. Don't know if this makes it better.
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.
An alternative would be to pass a terminalwriter object (_pytest.config.create_terminal_writer(self._config)) as the first arg of ColoredLevelFormatter. Don't know if this makes it better.
This reduces the number of dependencies at least (ColoredLevelFormatter
will then depend only on TerminalWriter
, instead of Config
and TerminalWriter
with the current code), so feel free to go with this route.
1ad52b9
to
ebab1b6
Compare
If all tests pass, this is ready for merging. |
Thanks @Thisch! |
I propose to prettify the default output of the live logs a bit by colorizing the levelnames.
A screenshot paints a thousand words: