flake8
-
reviewboard/notifications/email.py (Diff revision 1) Show all issues -
Review Request #9206 — Created Sept. 22, 2017 and submitted
The email generated after posting a review had an incomplete
'X-ReviewBoard-Diff-For' header. The header appeared for newly created files
and moved files but didn't appear for files that were only modified. The code
that was responsible for adding the header in question was ignoring modified
files. A check for modified files was added to the respective if statements and
a 'modified' attribute was added to the FileDiff class.
Posted reviews for a Tester Repository containing an edited file 'README.md'
and a created file 'FileA' to recreate the bug.Header generated from a posted review with both a created file and an edited
file:X-ReviewBoard-Diff-For: FileA
X-ReviewBoard-Diff-For
only appears for the created file, 'FileA'.After the fix, the same test was performed again with a created file 'FileA'
and an edited file 'README.md'.Header generated from a posted review with an edited file and a created file:
X-ReviewBoard-Diff-For: FileA X-ReviewBoard-Diff-For: README.md
X-ReviewBoard-Diff-For
appears for both 'FileA' and 'README.md'.Ran unit tests.
Description | From | Last Updated |
---|---|---|
The subject could also be improved by using the form: Fixed the X-ReviewBoard-Diff-For mail header to include modified files. This … |
chipx86 | |
Perhaps too much info in Testing Done. It's hard to see the meat of the testing with all the headers. … |
chipx86 | |
Descriptions and Testing Done should wrap at around 70-75 characters. The first line of the description can go away. The … |
chipx86 | |
This change will need unit tests. There's at least one existing test for this header. You should add a test … |
chipx86 | |
E501 line too long (90 > 79 characters) |
reviewbot | |
E501 line too long (89 > 79 characters) |
reviewbot | |
While the others don't have it, we're trying to introduce docstrings for every new function. These are always in the … |
chipx86 | |
We're now testing every single state except one in these cases. So instead, I'd suggest: if not filediff.is_new: and if … |
chipx86 | |
W293 blank line contains whitespace |
reviewbot | |
These blank lines should be removed. |
chipx86 | |
This isn't quite what we want. The function shouldn't be stating what it's doing internally (that's an implementation detail). Note … |
chipx86 | |
These blank lines should go away. |
chipx86 | |
No blank line. |
chipx86 | |
You can use: self.assertIn('X-ReviewBoard-Diff-For', message.rb_headers) |
chipx86 | |
I'd group the variable with the latter set of assertions, instead of the former. |
chipx86 | |
These should use self.assertIn. |
chipx86 | |
There should only be a single space between words. |
brennie | |
Can you add a comment explaining what this is testing and include the bug number? |
david | |
This should be a comment rather than part of the docstring (since the docstring is used for the test name). |
david | |
Sorry, one more thing. Let's move this into the body of the method, just below the docstring. |
david |
reviewboard/notifications/email.py (Diff revision 1) |
---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+10 -2) |
Perhaps too much info in Testing Done. It's hard to see the meat of the testing with all the headers. Keep it straightforward and to the point.
I'd also encourage using Markdown constructs more. You can use backticks for things like header names and class names, and triple backticks for blocks of literals like code samples or the blocks of e-mail headers.
Descriptions and Testing Done should wrap at around 70-75 characters.
The first line of the description can go away. The bug number should go in the Bugs field, and the Review Board version number (btw, "Review Board" and not "ReviewBoard") should just be a detail on the bug report (as it really applies to both older and newer versions than 2.0.30).
This change will need unit tests. There's at least one existing test for this header. You should add a test ensuring that all expected states are handled correctly.
reviewboard/diffviewer/models.py (Diff revision 2) |
---|
While the others don't have it, we're trying to introduce docstrings for every new function. These are always in the form of:
"""One-line summary."""
or:
"""One-line summary. Multi-line description. """
Function docstrings have some additional requirements (which are documented in Notion) but for properties, this is all you need. Search elsewhere in the codebase for
@property
and look for docstrings for examples.
reviewboard/notifications/email.py (Diff revision 2) |
---|
We're now testing every single state except one in these cases. So instead, I'd suggest:
if not filediff.is_new:
and
if not filediff.deleted:
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bugs: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+42 -2) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+42 -2) |
reviewboard/diffviewer/models.py (Diff revision 4) |
---|
This isn't quite what we want. The function shouldn't be stating what it's doing internally (that's an implementation detail). Note how it's just putting the logic of the code into words, which doesn't really help say what the purpose of this function is. It shouldn't even be documented as acting like a function, because as a property it's intended to be more like a value on a class.
Instead, it should say something like: "Whether this file is a modification to an existing file of the same name."
reviewboard/notifications/tests.py (Diff revision 4) |
---|
You can use:
self.assertIn('X-ReviewBoard-Diff-For', message.rb_headers)
reviewboard/notifications/tests.py (Diff revision 4) |
---|
I'd group the variable with the latter set of assertions, instead of the former.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+32 -2) |
reviewboard/diffviewer/models.py (Diff revision 5) |
---|
There should only be a single space between words.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+32 -2) |
reviewboard/notifications/tests.py (Diff revision 6) |
---|
Can you add a comment explaining what this is testing and include the bug number?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+36 -2) |
reviewboard/notifications/tests.py (Diff revision 7) |
---|
This should be a comment rather than part of the docstring (since the docstring is used for the test name).
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+35 -2) |
reviewboard/notifications/tests.py (Diff revision 8) |
---|
Sorry, one more thing. Let's move this into the body of the method, just below the docstring.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+35 -2) |