Enables the option for color to be displayed for warnings and errors.
Review Request #7704 — Created Oct. 16, 2015 and submitted
RBtools currently does not have the option for color to be displayed for warnings and errors.
I have changed the command-line UI, so that it defaults to color for warnings and errors, but
allows the user to disable the feature if they do not wish to use it.
I added logging.warning, logging.critical, and used a command that would give an error to see
if color would be displayed. After that I used the --no-color command to turn off the color off.
Also, I ran the unit tests and it passed them.
Description | From | Last Updated |
---|---|---|
Col: 25 E241 multiple spaces after ':' |
reviewbot | |
Col: 24 E241 multiple spaces after ':' |
reviewbot | |
Col: 27 E241 multiple spaces after ':' |
reviewbot | |
Col: 25 E241 multiple spaces after ':' |
reviewbot | |
Naming-wise, this should be NO_COLOR. However, it's best to have flags be more like "enabled" than "disabled". So, I'd prefer … |
chipx86 | |
Trailing period. You don't need to say "This feature." Instead, how about simply "Disables color output." |
chipx86 | |
I just know we're going to hit some situation where this looks bad on someone's system. How about allowing each … |
chipx86 | |
Blank line between these. |
chipx86 | |
Consider replacing this with handler.setFormatter(formatter) and then moving the colour checking up to where you initialize formatter. So we could … |
AD adriano | |
And these. |
chipx86 | |
Can you split this across multiple lines? |
chipx86 | |
These can probably go away now, right? |
david | |
This can still be all on one line. We should also keep it in alphabetical order. If we want to … |
david | |
Col: 17 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 17 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 17 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 21 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 21 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 21 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 21 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 21 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 21 E127 continuation line over-indented for visual indent |
reviewbot | |
This option group would ideally be a global thing. Currently, it is an option group that no command uses. What … |
brennie | |
--log-warn-color |
brennie | |
LOG_WARNING_COLOR |
brennie | |
--log-error-color |
brennie | |
LOG_ERROR_COLOR |
brennie | |
--log-critical-color |
brennie | |
LOG_CRITICAL_COLOR |
brennie | |
You also need to check if sys.stderr is a TTY. If it is not (e.g., it is a pipe or … |
brennie | |
You can do: handler.setFormatter(ColoredFormatter( '%(log_color)%s(levelname)-8s(reset)s', '...', ...)) |
brennie | |
Single quotes. |
brennie | |
self.options.warn_color |
brennie | |
Remvoe these. |
brennie | |
This won't work the way you expect. This alsod oesn't handle the case where stderr is a tty. You'll want … |
brennie | |
Blank line between these. |
brennie | |
This should be an else clause |
brennie | |
Cologlog doesnt work on windows without Colorama. We should check the platform and add colorama as a dep in that … |
brennie | |
I think make 'colorlog' and 'six>=1.8.0' in one line is better than two lines. |
XU xutong | |
I think what you meant to say was something like: if any(option in self.options.color for option in ('auto', 'always')): which … |
AD adriano | |
In comparison with the above field set, the fields in 'Option' are lined up with the opening parathesis. Should the … |
AH ahache | |
We always avoid backslashes if possible. Also, the logic is backwards a bit; if color is always, we always will … |
brennie | |
Let's reformat this to: install_requires = [ 'colorama', 'colorlog', 'six>=1.8.0', ] |
brennie | |
This should be an else clause otherwise this won't set up the formatter when the options are color == auto … |
brennie | |
Put the ] on the next line. Should end with a comma. |
brennie | |
No blank line here. |
brennie | |
Please format as: install_requires = [ 'colorama', 'colorlog', 'six>=1.8.0', ] |
brennie |
-
Tool: PEP8 Style Checker Processed Files: setup.py rbtools/commands/__init__.py Tool: Pyflakes Processed Files: setup.py rbtools/commands/__init__.py
-
-
rbtools/commands/__init__.py (Diff revision 2) We usually use single-quotes around our strings instead of double-quotes (see line 577), and it's generally recommended (although not always enforced) that we use trailing commas if we are listing a different argument on each line like this (see line 588). Also, see my below comment.
-
rbtools/commands/__init__.py (Diff revision 2) Consider replacing this with
handler.setFormatter(formatter)
and then moving the colour checking up to where you initializeformatter
. So we could replace the code starting at line 576 with:if self.options.nocolor: formatter = logging.Formatter('%(levelname)s: %(message)s') else: formatter = ColoredFormatter( '%(log_color)s%(levelname)-8s%(reset)s %(message)s', datefmt=None, reset=True, log_colors={ 'DEBUG': 'cyan', 'INFO': 'green', 'WARNING': 'yellow', 'ERROR': 'red,bg_black', 'CRITICAL': 'red,bg_white', }, secondary_log_colors={}, style='%', )
Also note how I used single quotes for the string instead of double quotes, as well as how I used a trailing comma for
style='%',
.
-
-
rbtools/commands/__init__.py (Diff revision 2) Naming-wise, this should be
NO_COLOR
.However, it's best to have flags be more like "enabled" than "disabled". So, I'd prefer having a
COLOR=False
thanNO_COLOR=True
.You can do this by changing the name, changing the action to
'store_false'
, and changing the default. -
rbtools/commands/__init__.py (Diff revision 2) Trailing period.
You don't need to say "This feature." Instead, how about simply "Disables color output."
-
rbtools/commands/__init__.py (Diff revision 2) I just know we're going to hit some situation where this looks bad on someone's system. How about allowing each of these to be customized through
.reviewboardrc
, with these as defaults? -
-
-
-
Tool: Pyflakes Processed Files: setup.py rbtools/commands/__init__.py Tool: PEP8 Style Checker Processed Files: setup.py rbtools/commands/__init__.py
-
-
setup.py (Diff revision 3) This can still be all on one line. We should also keep it in alphabetical order.
If we want to wrap it, let's do so like:
install_requires = [ 'colorlog', 'six>=1.8.0' ]
-
At this point, what's left to keep this as "WIP"?
I'm assuming that there are more parts to this project (like colorizing output of
rbt diff
andrbt status
), but I'd also assume those would be separate review requests. -
-
Tool: Pyflakes Processed Files: setup.py rbtools/commands/__init__.py Tool: PEP8 Style Checker Processed Files: setup.py rbtools/commands/__init__.py
-
rbtools/commands/__init__.py (Diff revision 4) Col: 17 E128 continuation line under-indented for visual indent
-
rbtools/commands/__init__.py (Diff revision 4) Col: 17 E128 continuation line under-indented for visual indent
-
rbtools/commands/__init__.py (Diff revision 4) Col: 17 E128 continuation line under-indented for visual indent
-
rbtools/commands/__init__.py (Diff revision 4) Col: 21 E127 continuation line over-indented for visual indent
-
rbtools/commands/__init__.py (Diff revision 4) Col: 21 E127 continuation line over-indented for visual indent
-
rbtools/commands/__init__.py (Diff revision 4) Col: 21 E127 continuation line over-indented for visual indent
-
rbtools/commands/__init__.py (Diff revision 4) Col: 21 E127 continuation line over-indented for visual indent
-
rbtools/commands/__init__.py (Diff revision 4) Col: 21 E127 continuation line over-indented for visual indent
-
rbtools/commands/__init__.py (Diff revision 4) Col: 21 E127 continuation line over-indented for visual indent
-
Tool: Pyflakes Processed Files: setup.py rbtools/commands/__init__.py Tool: PEP8 Style Checker Processed Files: setup.py rbtools/commands/__init__.py
-
-
rbtools/commands/__init__.py (Diff revision 5) This option group would ideally be a global thing. Currently, it is an option group that no command uses.
What happens when you put
color_options
in_global_options
? -
-
-
-
-
-
-
rbtools/commands/__init__.py (Diff revision 5) You also need to check if
sys.stderr
is a TTY. If it is not (e.g., it is a pipe or a file redirect), you don't want to send the escape sequences.See https://reviews.reviewboard.org/r/7716/diff/1#0.9 for an example of colorlog being used this way.
-
rbtools/commands/__init__.py (Diff revision 5) You can do:
handler.setFormatter(ColoredFormatter( '%(log_color)%s(levelname)-8s(reset)s', '...', ...))
-
-
Summary: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+51 -2) |
-
Tool: Pyflakes Processed Files: setup.py rbtools/commands/__init__.py Tool: PEP8 Style Checker Processed Files: setup.py rbtools/commands/__init__.py
-
-
rbtools/commands/__init__.py (Diff revision 6) This won't work the way you expect. This alsod oesn't handle the case where
stderr
is a tty.You'll want to do:
if (self.options.color == 'always' or (self.options.color == 'auto' and not sys.stderr.isatty())):
-
-
-
setup.py (Diff revision 6) Cologlog doesnt work on windows without Colorama. We should check the platform and add colorama as a dep in that case.
-
Tool: Pyflakes Processed Files: setup.py rbtools/commands/__init__.py Tool: PEP8 Style Checker Processed Files: setup.py rbtools/commands/__init__.py
-
-
setup.py (Diff revision 7) I think make 'colorlog' and 'six>=1.8.0' in one line is better than two lines.
-
Tool: Pyflakes Processed Files: setup.py rbtools/commands/__init__.py Tool: PEP8 Style Checker Processed Files: setup.py rbtools/commands/__init__.py
-
-
rbtools/commands/__init__.py (Diff revision 8) I think what you meant to say was something like:
if any(option in self.options.color for option in ('auto', 'always')):
which is a fancier version of:
if 'auto' in self.options.color or 'always' in self.options.color:
-
Tool: PEP8 Style Checker Processed Files: setup.py rbtools/commands/__init__.py Tool: Pyflakes Processed Files: setup.py rbtools/commands/__init__.py
-
-
rbtools/commands/__init__.py (Diff revision 9) In comparison with the above field set, the fields in 'Option' are lined up with the opening parathesis. Should the fields for OptionGroup also be indented accordingly? or does that make every line exceed the 80 character limit?
-
-
rbtools/commands/__init__.py (Diff revision 9) We always avoid backslashes if possible. Also, the logic is backwards a bit; if
color
isalways
, we always will usecolorlog
. Otherwise, if it isauto
and we are using a TTY, then we will sue it too.This can be reformatted as:
if (self.options.color == 'always' or (self.options.color == 'auto' and sys.stderr.isatty())):
-
setup.py (Diff revision 9) Let's reformat this to:
install_requires = [ 'colorama', 'colorlog', 'six>=1.8.0', ]
-
Tool: Pyflakes Processed Files: setup.py rbtools/commands/__init__.py Tool: PEP8 Style Checker Processed Files: setup.py rbtools/commands/__init__.py
-
-
rbtools/commands/__init__.py (Diff revision 10) This should be an
else
clause otherwise this won't set up the formatter when the options arecolor == auto
and stderr is not a tty. -
-
Tool: Pyflakes Processed Files: setup.py rbtools/commands/__init__.py Tool: PEP8 Style Checker Processed Files: setup.py rbtools/commands/__init__.py
-
Minor nitpicks. Looks good to me!
-
-
setup.py (Diff revision 11) Please format as:
install_requires = [ 'colorama', 'colorlog', 'six>=1.8.0', ]
-
Tool: Pyflakes Processed Files: setup.py rbtools/commands/__init__.py Tool: PEP8 Style Checker Processed Files: setup.py rbtools/commands/__init__.py