Add user-configurable option to suppress git, mercurial, and perforce warnings.

Review Request #8638 — Created Jan. 20, 2017 and submitted

szhang
RBTools
master
4041
0f8e84c...
rbtools, students

Prior to this change, it was not possible for the user to suppress warnings from git, mercurial, and perforce. This could result in a large number of warning messages output to the log which can be cumbersome and fill up the console window.

This change allows the user to suppress warnings from rbtools/clients/git.py, rbtools/clients/mercurial.py, and rbtools/clients/perforce.py by setting SUPPRESS_CLIENT_WARNINGS in .reviewboardrc to True.

Checked that the warning message in git.py is suppressed when the SUPPRESS_CLIENT_WARNINGS is set to True.

Description From Last Updated

Probably unintentional in the description: "It was not possible not ..."

chipx86chipx86

Col: 80 E501 line too long (100 > 79 characters)

reviewbotreviewbot

This could be fixed like: if (self.has_pending_changes() and not self.config.get('SUPPRESS_CLIENT_WARNINGS', True)): The default value for SUPPRESS_CLIENT_WARNINGS should be False.

daviddavid

Same comments here as in git.py

daviddavid

Col: 80 E501 line too long (104 > 79 characters)

reviewbotreviewbot

Fix the default value here and below.

daviddavid

After the conditional block, please add a blank line. Here and below.

daviddavid

When spanning multiple lines, use parens instead of \.

chipx86chipx86

No need for parens around the self.config.get. There's no other logic happening in here that needs to be grouped.

chipx86chipx86

Col: 74 W291 trailing whitespace

reviewbotreviewbot

This should also use parens.

chipx86chipx86

I think the main reason for the change is to suppress the ones that are more.. hmm, noisy and meaningless? ...

chipx86chipx86

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

I don't think we need the change to this comment (since the code is relatively self-explanatory), but can we make ...

daviddavid

Mind adding a period at the end of this comment?

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
    
    
  2. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (100 > 79 characters)
    
  3. rbtools/clients/mercurial.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (104 > 79 characters)
    
  4. 
      
david
  1. 
      
  2. rbtools/clients/git.py (Diff revision 1)
     
     

    This could be fixed like:

    if (self.has_pending_changes() and
        not self.config.get('SUPPRESS_CLIENT_WARNINGS', True)):
    

    The default value for SUPPRESS_CLIENT_WARNINGS should be False.

  3. rbtools/clients/mercurial.py (Diff revision 1)
     
     

    Same comments here as in git.py

  4. rbtools/clients/perforce.py (Diff revision 1)
     
     

    Fix the default value here and below.

  5. rbtools/clients/perforce.py (Diff revision 1)
     
     
     

    After the conditional block, please add a blank line. Here and below.

  6. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
    
    
  2. rbtools/clients/git.py (Diff revision 2)
     
     
    Col: 74
     W291 trailing whitespace
    
  3. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  5. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  7. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  8. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  9. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  10. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  11. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  12. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  13. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  14. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  15. 
      
chipx86
  1. 
      
  2. Probably unintentional in the description: "It was not possible not ..."

  3. rbtools/clients/git.py (Diff revision 2)
     
     
     

    When spanning multiple lines, use parens instead of \.

  4. rbtools/clients/git.py (Diff revision 2)
     
     

    No need for parens around the self.config.get. There's no other logic happening in here that needs to be grouped.

  5. rbtools/clients/mercurial.py (Diff revision 2)
     
     
     

    This should also use parens.

  6. rbtools/clients/perforce.py (Diff revision 2)
     
     
     
     

    I think the main reason for the change is to suppress the ones that are more.. hmm, noisy and meaningless? Like the working directory warnings, which are safe and fine and a person might be well aware.

    Ones about skipping files during diff building are probably more important. I'd want to get David and Barret's take on this, but I think we should probably keep these. They'll only show up when it's more important. Could be a sign that their review request won't turn out right.

    If we do keep these, then I suggest storing the result of that config value in __init__ and referring to that, instead of doing a more costly/verbose check every time.

    1. So I had glanced at these and determined that we could suppress them, but I was on the fence about the perforce ones. I figured if someone is using p4 and wants to use this config value, it's these warnings that they were wanting to suppress.

  7. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
    
    
  2. 
      
david
  1. 
      
  2. rbtools/clients/git.py (Diff revision 3)
     
     
     

    I don't think we need the change to this comment (since the code is relatively self-explanatory), but can we make sure the comment ends in a period?

  3. rbtools/clients/mercurial.py (Diff revision 3)
     
     

    Mind adding a period at the end of this comment?

  4. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
szhang
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.7.x (e0e4080)
Loading...