Enables the option for color to be displayed for warnings and errors.

Review Request #7704 — Created Oct. 16, 2015 and submitted

Information

RBTools
master

Reviewers

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 ':'

reviewbotreviewbot

Col: 24 E241 multiple spaces after ':'

reviewbotreviewbot

Col: 27 E241 multiple spaces after ':'

reviewbotreviewbot

Col: 25 E241 multiple spaces after ':'

reviewbotreviewbot

Naming-wise, this should be NO_COLOR. However, it's best to have flags be more like "enabled" than "disabled". So, I'd prefer …

chipx86chipx86

Trailing period. You don't need to say "This feature." Instead, how about simply "Disables color output."

chipx86chipx86

I just know we're going to hit some situation where this looks bad on someone's system. How about allowing each …

chipx86chipx86

Blank line between these.

chipx86chipx86

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.

chipx86chipx86

Can you split this across multiple lines?

chipx86chipx86

These can probably go away now, right?

daviddavid

This can still be all on one line. We should also keep it in alphabetical order. If we want to …

daviddavid

Col: 17 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 17 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 17 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

This option group would ideally be a global thing. Currently, it is an option group that no command uses. What …

brenniebrennie

--log-warn-color

brenniebrennie

LOG_WARNING_COLOR

brenniebrennie

--log-error-color

brenniebrennie

LOG_ERROR_COLOR

brenniebrennie

--log-critical-color

brenniebrennie

LOG_CRITICAL_COLOR

brenniebrennie

You also need to check if sys.stderr is a TTY. If it is not (e.g., it is a pipe or …

brenniebrennie

You can do: handler.setFormatter(ColoredFormatter( '%(log_color)%s(levelname)-8s(reset)s', '...', ...))

brenniebrennie

Single quotes.

brenniebrennie

self.options.warn_color

brenniebrennie

Remvoe these.

brenniebrennie

This won't work the way you expect. This alsod oesn't handle the case where stderr is a tty. You'll want …

brenniebrennie

Blank line between these.

brenniebrennie

This should be an else clause

brenniebrennie

Cologlog doesnt work on windows without Colorama. We should check the platform and add colorama as a dep in that …

brenniebrennie

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 …

brenniebrennie

Let's reformat this to: install_requires = [ 'colorama', 'colorlog', 'six>=1.8.0', ]

brenniebrennie

This should be an else clause otherwise this won't set up the formatter when the options are color == auto …

brenniebrennie

Put the ] on the next line. Should end with a comma.

brenniebrennie

No blank line here.

brenniebrennie

Please format as: install_requires = [ 'colorama', 'colorlog', 'six>=1.8.0', ]

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. rbtools/commands/__init__.py (Diff revision 1)
     
     
    Col: 25
     E241 multiple spaces after ':'
    
  3. rbtools/commands/__init__.py (Diff revision 1)
     
     
    Col: 24
     E241 multiple spaces after ':'
    
  4. rbtools/commands/__init__.py (Diff revision 1)
     
     
    Col: 27
     E241 multiple spaces after ':'
    
  5. rbtools/commands/__init__.py (Diff revision 1)
     
     
    Col: 25
     E241 multiple spaces after ':'
    
  6. 
      
DA
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. 
      
DA
AD
  1. 
      
  2. 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.

  3. rbtools/commands/__init__.py (Diff revision 2)
     
     
     
     
     

    Consider replacing this with handler.setFormatter(formatter) and then moving the colour checking up to where you initialize formatter. 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='%',.

    1. We use trailing commas for lists and tuples, but not for arguments to a function call. The idea being that we often add something to the end of a list or tuple, but changes to arguments are often much more context-specific.

    2. Ah, my mistake. Disregard my trailing comma comment.

  4. 
      
chipx86
  1. 
      
  2. 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 than NO_COLOR=True.

    You can do this by changing the name, changing the action to 'store_false', and changing the default.

  3. rbtools/commands/__init__.py (Diff revision 2)
     
     

    Trailing period.

    You don't need to say "This feature." Instead, how about simply "Disables color output."

  4. 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?

  5. rbtools/commands/__init__.py (Diff revision 2)
     
     
     

    Blank line between these.

  6. rbtools/commands/__init__.py (Diff revision 2)
     
     
     

    And these.

  7. setup.py (Diff revision 2)
     
     

    Can you split this across multiple lines?

  8. 
      
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. 
      
david
  1. 
      
  2. 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'
    ]
    
  3. 
      
david
  1. 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 and rbt status), but I'd also assume those would be separate review requests.

    1. Christian mentioned allowing the colors to be customized through .reviewboardrc

  2. rbtools/commands/__init__.py (Diff revision 3)
     
     
     
     

    These can probably go away now, right?

  3. 
      
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. rbtools/commands/__init__.py (Diff revision 4)
     
     
    Col: 17
     E128 continuation line under-indented for visual indent
    
  3. rbtools/commands/__init__.py (Diff revision 4)
     
     
    Col: 17
     E128 continuation line under-indented for visual indent
    
  4. rbtools/commands/__init__.py (Diff revision 4)
     
     
    Col: 17
     E128 continuation line under-indented for visual indent
    
  5. rbtools/commands/__init__.py (Diff revision 4)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  6. rbtools/commands/__init__.py (Diff revision 4)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  7. rbtools/commands/__init__.py (Diff revision 4)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  8. rbtools/commands/__init__.py (Diff revision 4)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  9. rbtools/commands/__init__.py (Diff revision 4)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  10. rbtools/commands/__init__.py (Diff revision 4)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  11. 
      
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. 
      
brennie
  1. 
      
  2. 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 ?

  3. rbtools/commands/__init__.py (Diff revision 5)
     
     

    --log-warn-color

  4. rbtools/commands/__init__.py (Diff revision 5)
     
     

    LOG_WARNING_COLOR

  5. rbtools/commands/__init__.py (Diff revision 5)
     
     

    --log-error-color

  6. rbtools/commands/__init__.py (Diff revision 5)
     
     

    LOG_ERROR_COLOR

  7. rbtools/commands/__init__.py (Diff revision 5)
     
     

    --log-critical-color

  8. rbtools/commands/__init__.py (Diff revision 5)
     
     

    LOG_CRITICAL_COLOR

  9. 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.

  10. rbtools/commands/__init__.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    You can do:

    handler.setFormatter(ColoredFormatter(
        '%(log_color)%s(levelname)-8s(reset)s',
        '...',
        ...))
    
  11. rbtools/commands/__init__.py (Diff revision 5)
     
     
     

    Single quotes.

  12. rbtools/commands/__init__.py (Diff revision 5)
     
     
     
     
     

    Remvoe these.

  13. 
      
brennie
  1. 
      
  2. rbtools/commands/__init__.py (Diff revision 5)
     
     

    self.options.warn_color

  3. etc.

DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. 
      
brennie
  1. 
      
  2. 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())):
    
  3. rbtools/commands/__init__.py (Diff revision 6)
     
     
     

    Blank line between these.

  4. rbtools/commands/__init__.py (Diff revision 6)
     
     

    This should be an else clause

  5. 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.

  6. 
      
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. 
      
XU
  1. 
      
  2. setup.py (Diff revision 7)
     
     

    I think make 'colorlog' and 'six>=1.8.0' in one line is better than two lines.

    1. It should be on separate lines.

  3. 
      
brennie
  1. Your latest diff doesn't contain any changes.

  2. 
      
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. 
      
AD
  1. 
      
  2. 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:
    
    1. While you're right that there's a bug here, the second option you present (the 'less fancy' one) is preferable. If there were a few dozen options, we might change our mind, but for just two, it's better to be simple and explicit.

    2. I actually changed this, but I guess when I reset my branches the changes I made were erased. Updated the post again.

  3. 
      
DA
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. 
      
AH
  1. 
      
  2. 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?

    1. Some of the lines would go over the 80 line limit, and I don't think python would let me have different indentations

  3. 
      
brennie
  1. 
      
  2. rbtools/commands/__init__.py (Diff revision 9)
     
     
     

    We always avoid backslashes if possible. Also, the logic is backwards a bit; if color is always, we always will use colorlog. Otherwise, if it is auto 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())):
    
  3. setup.py (Diff revision 9)
     
     
     
     

    Let's reformat this to:

    install_requires = [
        'colorama',
        'colorlog',
        'six>=1.8.0',
    ]
    
  4. 
      
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/__init__.py (Diff revision 10)
     
     

    This should be an else clause otherwise this won't set up the formatter when the options are color == auto and stderr is not a tty.

  3. setup.py (Diff revision 10)
     
     

    Put the ] on the next line.

    Should end with a comma.

  4. 
      
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. 
      
brennie
  1. Minor nitpicks. Looks good to me!

  2. rbtools/commands/__init__.py (Diff revision 11)
     
     

    No blank line here.

  3. setup.py (Diff revision 11)
     
     
     
     
     

    Please format as:

    install_requires = [
        'colorama',
        'colorlog',
        'six>=1.8.0',
    ]
    
  4. 
      
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. 
      
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
        rbtools/commands/__init__.py
    
    
  2. 
      
DA
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (126393c)
Loading...