Summary

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

Review Request #8638 — Created Jan. 20, 2017 and submitted March 5, 2017, 5:20 p.m.

Information

Owner

szhang

Repository

RBTools

Branch

master

Bugs

4041

Depends On

Commit

0f8e84c...

Reviewers

Groups

rbtools, students

People

Description

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.

Testing Done

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

Issues

Description	From	Last Updated
Probably unintentional in the description: "It was not possible not ..."	chipx86	Jan. 24, 2017, 10:54 a.m.
Col: 80 E501 line too long (100 > 79 characters)	reviewbot	Jan. 20, 2017, 6:53 p.m.
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.	david	Jan. 21, 2017, 9:57 a.m.
Same comments here as in git.py	david	Jan. 20, 2017, 7:01 p.m.
Col: 80 E501 line too long (104 > 79 characters)	reviewbot	Jan. 21, 2017, 9:49 a.m.
Fix the default value here and below.	david	Jan. 21, 2017, 9:57 a.m.
After the conditional block, please add a blank line. Here and below.	david	Jan. 20, 2017, 7:01 p.m.
When spanning multiple lines, use parens instead of \.	chipx86	Jan. 24, 2017, 10:58 a.m.
No need for parens around the self.config.get. There's no other logic happening in here that needs to be grouped.	chipx86	Jan. 24, 2017, 10:58 a.m.
Col: 74 W291 trailing whitespace	reviewbot	Jan. 24, 2017, 11 a.m.
This should also use parens.	chipx86	Jan. 24, 2017, 10:59 a.m.
I think the main reason for the change is to suppress the ones that are more.. hmm, noisy and meaningless? …	chipx86	Feb. 8, 2017, 6:33 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:02 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:02 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:02 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:02 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:03 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:03 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:03 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:03 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:04 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:04 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:04 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 24, 2017, 11:04 a.m.
I don't think we need the change to this comment (since the code is relatively self-explanatory), but can we make …	david	March 4, 2017, 7:10 p.m.
Mind adding a period at the end of this comment?	david	March 4, 2017, 7:10 p.m.

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

rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (100 > 79 characters)
```
rbtools/clients/mercurial.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (104 > 79 characters)
```

rbtools/clients/git.py (Diff revision 1)

The issue has been resolved. Show all issues

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.

rbtools/clients/mercurial.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same comments here as in git.py
```
rbtools/clients/perforce.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Fix the default value here and below.
```
rbtools/clients/perforce.py (Diff revision 1)
The issue has been resolved. Show all issues
```
After the conditional block, please add a blank line. Here and below.
```

Change Summary:

Addressed the issues raised by reviewbot and david.

Changed the checks to not (self.config.get('SUPPRESS_CLIENT_WARNINGS', False) as this would act as if SUPPRESS_CLIENT_WARNINGS is set to False. This effectively makes the default value set to False as david mentioned.

Commit:

c2a5697b62caae79c0c647a80e150ec37abd46f2

2e2e8451e11cd8d714039f5829aba0e7b96db659

Diff:

Revision 2 (+49 -22)

Show changes

	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py

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

rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 74
 W291 trailing whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

The issue has been resolved. Show all issues

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

rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
When spanning multiple lines, use parens instead of \.
```

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

The issue has been resolved. Show all issues

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

rbtools/clients/mercurial.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should also use parens.
```

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

The issue has been dropped. Show all issues

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.

david Jan. 21, 2017, 10:20 a.m.

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.

Change Summary:

Addresses the issues with the style of the code (eg. whitespace on blank lines)

Description:

~		Prior to this change, it was not possible not 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.
	~	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.

Commit:

2e2e8451e11cd8d714039f5829aba0e7b96db659

27042ecf8f6bc642e4e17bd3bb308fb34beea3a4

Diff:

Revision 3 (+49 -22)

Show changes

	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py

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

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

The issue has been resolved. Show all issues

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?

rbtools/clients/mercurial.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Mind adding a period at the end of this comment?
```

Change Summary:

Fix issues raised by David.

Commit:

27042ecf8f6bc642e4e17bd3bb308fb34beea3a4

0f8e84c8696b12cf498fe157e53d06e6e327b7bc

Diff:

Revision 4 (+50 -23)

Show changes

	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-0.7.x (e0e4080)