Add user-configurable option to suppress git, mercurial, and perforce warnings.
Review Request #8638 — Created Jan. 20, 2017 and submitted
| Information | |
|---|---|
| szhang | |
| RBTools | |
| master | |
| 4041 | |
| 0f8e84c... | |
| Reviewers | |
| 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 ..." |
 chipx86
|
|
|
Col: 80 E501 line too long (100 > 79 characters) |
 reviewbot
|
|
|
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
|
|
|
Same comments here as in git.py |
david
|
|
|
Col: 80 E501 line too long (104 > 79 characters) |
reviewbot
|
|
|
Fix the default value here and below. |
david
|
|
|
After the conditional block, please add a blank line. Here and below. |
david
|
|
|
When spanning multiple lines, use parens instead of \. |
chipx86
|
|
|
No need for parens around the self.config.get. There's no other logic happening in here that needs to be grouped. |
chipx86
|
|
|
Col: 74 W291 trailing whitespace |
reviewbot
|
|
|
This should also use parens. |
chipx86
|
|
|
I think the main reason for the change is to suppress the ones that are more.. hmm, noisy and meaningless? … |
chipx86
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
I don't think we need the change to this comment (since the code is relatively self-explanatory), but can we make … |
david
|
|
|
Mind adding a period at the end of this comment? |
david
|
-
-
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_WARNINGSshould beFalse. -
-
-
rbtools/clients/perforce.py (Diff revision 1) 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 ifSUPPRESS_CLIENT_WARNINGSis set toFalse. This effectively makes the default value set toFalseas david mentioned.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+49 -22) |
-
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) No need for parens around the
self.config.get. There's no other logic happening in here that needs to be grouped. -
-
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.
Change Summary:
Addresses the issues with the style of the code (eg. whitespace on blank lines)
Description: |
|
||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||
Diff: |
Revision 3 (+49 -22) |
-
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) 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?
-
Change Summary:
Fix issues raised by David.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+50 -23) |
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