-
-
-
-
reviewboard/notifications/email.py (Diff revision 1) Col: 1 E101 indentation contains mixed spaces and tabs
-
-
-
-
-
-
reviewboard/notifications/email.py (Diff revision 1) Col: 80 E501 line too long (83 > 79 characters)
-
-
reviewboard/notifications/email.py (Diff revision 1) local variable 'counter' is assigned to but never used
-
-
-
-
-
-
Added cap to filename header
Review Request #8646 — Created Jan. 20, 2017 and submitted
Clients occasionally dump a ton of data in the filename e-mail headers, which many SMTP servers reject.
To combat this, I have added a limit to the number of characters (8192 characters) that these filename headers may contain. Once this limit is reached, additional filename headers are not added.
Sent e-mail through gmail servers; cap worked correctly.
Unit test was added that checks for correct logger warning, as well as testing the correct overflow behavior.
Description | From | Last Updated |
---|---|---|
Does this have a bug number? |
david | |
Please flesh out your description to include more info on what the bug was and how you fixed it. See … |
david | |
We're also going to need a unit test that tests overflowing this header and checking for warnings. One of the … |
chipx86 | |
Your summary should mention that it is an e-mail header because HTTP also has headers. |
brennie | |
'formataddr' imported but unused |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 E101 indentation contains mixed spaces and tabs |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 80 E501 line too long (83 > 79 characters) |
reviewbot | |
undefined name 'counter' |
reviewbot | |
local variable 'counter' is assigned to but never used |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot | |
Col: 29 E701 multiple statements on one line (colon) |
reviewbot | |
Col: 13 E701 multiple statements on one line (colon) |
reviewbot | |
Col: 9 E122 continuation line missing indentation or outdented |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot | |
Col: 29 E701 multiple statements on one line (colon) |
reviewbot | |
Col: 13 E701 multiple statements on one line (colon) |
reviewbot | |
Col: 9 E122 continuation line missing indentation or outdented |
reviewbot | |
It looks like your editor created this. Please remove it from your change. |
david | |
'formataddr' imported but unused |
reviewbot | |
Please put a blank line between these. |
david | |
Please add a comment explaining why we do this complex stuff. |
david | |
We should iterate this as for key, value in six.iteritems(headers): |
david | |
These lines could all be combined. |
david | |
Please put a blank line between these. |
david | |
We should have a comment explaining what this "2" is for. We should also include some extra padding in here … |
david | |
It might be nice to pull out 'X-ReviewBoard-Diff-For' into a constant string. We can also compute the length of it … |
david | |
Please put a blank line between these. |
david | |
It might be nice to pull this out into a constant. |
david | |
This could be current_header_length = newlength, since we already did that same computation. |
david | |
After a block, please put a newline. Can we add a log statement to the break case to record that … |
david | |
We only want 2 blank lines between sections at the top level of a file. |
chipx86 | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 5 E266 too many leading '#' for block comment |
reviewbot | |
Col: 5 E303 too many blank lines (3) |
reviewbot | |
Col: 80 E501 line too long (123 > 79 characters) |
reviewbot | |
Only one # for these. Make sure to wrap at <= 79 characters. |
chipx86 | |
Col: 5 E266 too many leading '#' for block comment |
reviewbot | |
Col: 80 E501 line too long (115 > 79 characters) |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
I suggest storing this as a constant at the top-level of the module. Constants are also generally uppercase. This one … |
chipx86 | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 9 E266 too many leading '#' for block comment |
reviewbot | |
Col: 80 E501 line too long (106 > 79 characters) |
reviewbot | |
\r\n, not /r/n. |
chipx86 | |
Col: 9 E266 too many leading '#' for block comment |
reviewbot | |
So-called "magic numbers" (the 6) should always have a constant. |
chipx86 | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 5 E303 too many blank lines (2) |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 9 E122 continuation line missing indentation or outdented |
reviewbot | |
No blank line here. |
chipx86 | |
Col: 56 E701 multiple statements on one line (colon) |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
No blank line. |
chipx86 | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 9 E122 continuation line missing indentation or outdented |
reviewbot | |
Col: 13 E701 multiple statements on one line (colon) |
reviewbot | |
Col: 80 E501 line too long (103 > 79 characters) |
reviewbot | |
Make sure to test this. There's no logger. You also need to provide more information in the log message. As … |
chipx86 | |
Col: 80 E501 line too long (120 > 79 characters) |
reviewbot | |
Not sure we want to put a header in here for this? I don't know whether Warning is a standard … |
chipx86 | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
'formataddr' imported but unused |
reviewbot | |
Can you document these? Refer to https://www.notion.so/reviewboard/Writing-Python-Docs-845c696271194d85b69503246db1d0e6#574a663ebae8453b8b014bd9ad841ce3 |
chipx86 | |
Comments should wrap at around character 79. |
chipx86 | |
No blank line here. |
chipx86 | |
No parens needed. |
chipx86 | |
This should also explicitly say that this is when sending e-mail. Something like: "Unable to store all filenames in X-ReviewBoard-Diff-For … |
chipx86 | |
'formataddr' imported but unused |
reviewbot | |
Almost there, but not quite right. This should be using the #: syntax as shown in my link, and there … |
chipx86 | |
I have a couple comments in this code block, but they're more to help learn about our code style. I … |
chipx86 | |
You can fit more per line. |
chipx86 | |
These should be the same statement. If you need to wrap lines, use parens, like: current_header_length += (len(key) + len(value) … |
chipx86 | |
I would actually reverse this. Try: if current_header_length >= DIFF_FOR_HEADER_MAX_LENGTH: logging... break headers.appendlist(...) |
chipx86 | |
Two important things here: 1) This isn't an error, it's a warning. We should use logging.warning. 2) """ should never … |
chipx86 | |
You don't need to (and shouldn't do) str(...). The %s will take care of it automatically. |
chipx86 | |
Col: 72 W291 trailing whitespace |
reviewbot | |
Col: 68 E226 missing whitespace around arithmetic operator |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 70 W291 trailing 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 | |
Blank line between these two. Can we also call the constant MAX_FILENAME_HEADERS_LENGTH? |
david | |
This should just be a regular string, not a raw string (no 'r' prefix). It should also use single quotes. |
david | |
typos: "headrs", "addin". There's also an extra space at the beginning of this line. |
david | |
extra "g" that got chopped off from the previous line. |
david | |
This should wrap on word boundaries rather than breaking words: logging.warning('Unable to store all filenames in ' 'X-ReviewBoard-Diff-For headers when … |
david | |
logging.X() will do a format operation, so these can be passed in as additional arguments rather than doing your own … |
david | |
Add a blank line between these two. |
david | |
local variable 'from_email' is assigned to but never used |
reviewbot | |
Please use single-quotes around 'X' |
david | |
local variable 'filediff' is assigned to but never used |
reviewbot | |
Leftover debug code? |
david | |
Please wrap the string on word boundaries. |
david | |
local variable 'from_email' is assigned to but never used |
reviewbot | |
local variable 'filediff' is assigned to but never used |
reviewbot | |
Col: 33 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 33 E128 continuation line under-indented for visual indent |
reviewbot | |
The contents of the docstring should be in the doc comments below. No sense splitting it up or repeating it. |
brennie | |
Looks like it got a bit mangled at the end. |
brennie | |
This should not be a raw string. This will be a string that is four characters long: >>> r'\r\n' '\\r\\n' … |
brennie | |
Colon instead of semicolon. |
brennie | |
This will take less lines and be easier to read as: logging.warning( '.... ... ...' '... ... ...', ...) |
brennie | |
Undo this change. |
brennie | |
So we are going to generate a 100 character string 400 times instead of just doing it once. Instead we … |
brennie | |
Can we just rename this filename? |
brennie | |
If we are always adding at least one character and we are truncating to the last 100, we can technically … |
brennie | |
"and" should not be split over two lines. |
brennie | |
Colon instead of semicolon here. |
brennie | |
You shouldn't need a u' prefix. We use from __future__ import unicode_literals at the start of each file, which turns … |
brennie | |
spy.called is equivalent to len(spy.calls) > 0. If spy.called is False the above assertions will raise an exception. This should … |
brennie | |
Since this is a raw string, it will have length 4. Do not use a raw string (i.e., remove the … |
brennie | |
Undo this change. The line is unnecessary but should be fixed in a different patch. |
brennie | |
local variable 'from_email' is assigned to but never used |
reviewbot | |
Col: 26 E703 statement ends with a semicolon |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
So str is not actually the string type for Python 2: unicode is. However, using %-formatting will call the relevant … |
brennie | |
Col: 49 W291 trailing whitespace |
reviewbot | |
Col: 70 W291 trailing whitespace |
reviewbot | |
Col: 77 W291 trailing whitespace |
reviewbot | |
You don't need the backticks in comments because they will never be rendered. |
brennie | |
'formataddr' imported but unused |
reviewbot | |
Col: 33 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 33 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 33 E128 continuation line under-indented for visual indent |
reviewbot | |
local variable 'from_email' is assigned to but never used |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 49 W291 trailing whitespace |
reviewbot | |
Col: 70 W291 trailing whitespace |
reviewbot | |
Col: 77 W291 trailing whitespace |
reviewbot | |
This should just be a comment -- it needn't be a doc comment (#:). |
brennie | |
Can you surround X-Reviewboard-Diff-For with double backticks? |
brennie | |
Likewise here? |
brennie | |
And the whole header thing here too. |
brennie | |
local variable 'from_email' is assigned to but never used |
reviewbot | |
This comment isn't entirely correct (we no longer have DIFF_FOR_HEADER_MAX_LENGTH), plus it's duplicating stuff on the individual doc comments for … |
david | |
There's an extra set of parens around the log text that can be removed. |
david | |
local variable 'from_email' is assigned to but never used |
reviewbot | |
Did you run this code? The %ss should have been filled in. |
david | |
You can fit more per line. |
chipx86 | |
Blank line before comments. |
chipx86 | |
The ordering is a bit weird. I'd kinda suggest comparing the entirety of args at once. |
chipx86 |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+23 -2) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py Ignored Files: .project
-
reviewboard/notifications/email.py (Diff revision 2) Col: 80 E501 line too long (86 > 79 characters)
-
reviewboard/notifications/email.py (Diff revision 2) Col: 29 E701 multiple statements on one line (colon)
-
reviewboard/notifications/email.py (Diff revision 2) Col: 13 E701 multiple statements on one line (colon)
-
reviewboard/notifications/email.py (Diff revision 2) Col: 9 E122 continuation line missing indentation or outdented
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+22 -2) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py Ignored Files: .project
-
reviewboard/notifications/email.py (Diff revision 3) Col: 80 E501 line too long (86 > 79 characters)
-
reviewboard/notifications/email.py (Diff revision 3) Col: 29 E701 multiple statements on one line (colon)
-
reviewboard/notifications/email.py (Diff revision 3) Col: 13 E701 multiple statements on one line (colon)
-
reviewboard/notifications/email.py (Diff revision 3) Col: 9 E122 continuation line missing indentation or outdented
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+23 -2) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py Ignored Files: .project Tool: Pyflakes Processed Files: reviewboard/notifications/email.py Ignored Files: .project
-
-
-
-
Please flesh out your description to include more info on what the bug was and how you fixed it. See https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/ for a guide.
-
.project (Diff revision 4) It looks like your editor created this. Please remove it from your change.
-
-
reviewboard/notifications/email.py (Diff revision 4) Please add a comment explaining why we do this complex stuff.
-
reviewboard/notifications/email.py (Diff revision 4) We should iterate this as
for key, value in six.iteritems(headers):
-
-
-
reviewboard/notifications/email.py (Diff revision 4) We should have a comment explaining what this "2" is for. We should also include some extra padding in here to accomodate newlines (can you check to see if emails use \r\n or just \n?)
-
reviewboard/notifications/email.py (Diff revision 4) It might be nice to pull out 'X-ReviewBoard-Diff-For' into a constant string. We can also compute the length of it once, outside the loop, and then just add the length of the filename inside.
-
-
reviewboard/notifications/email.py (Diff revision 4) It might be nice to pull this out into a constant.
-
reviewboard/notifications/email.py (Diff revision 4) This could be
current_header_length = newlength
, since we already did that same computation. -
reviewboard/notifications/email.py (Diff revision 4) After a block, please put a newline.
Can we add a log statement to the
break
case to record that it was clipped? It might be nice to also include a header in this case explaining that the file list is clipped.
Description: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Bugs: |
|
||||||||||||
Commit: |
|
||||||||||||
Diff: |
Revision 5 (+28 -2) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py
-
-
-
reviewboard/notifications/email.py (Diff revision 5) Col: 5 E266 too many leading '#' for block comment
-
-
reviewboard/notifications/email.py (Diff revision 5) Col: 80 E501 line too long (123 > 79 characters)
-
reviewboard/notifications/email.py (Diff revision 5) Col: 5 E266 too many leading '#' for block comment
-
reviewboard/notifications/email.py (Diff revision 5) Col: 80 E501 line too long (115 > 79 characters)
-
-
-
reviewboard/notifications/email.py (Diff revision 5) Col: 9 E266 too many leading '#' for block comment
-
reviewboard/notifications/email.py (Diff revision 5) Col: 80 E501 line too long (106 > 79 characters)
-
reviewboard/notifications/email.py (Diff revision 5) Col: 9 E266 too many leading '#' for block comment
-
-
-
-
-
-
reviewboard/notifications/email.py (Diff revision 5) Col: 9 E122 continuation line missing indentation or outdented
-
reviewboard/notifications/email.py (Diff revision 5) Col: 56 E701 multiple statements on one line (colon)
-
-
-
reviewboard/notifications/email.py (Diff revision 5) Col: 9 E122 continuation line missing indentation or outdented
-
reviewboard/notifications/email.py (Diff revision 5) Col: 13 E701 multiple statements on one line (colon)
-
reviewboard/notifications/email.py (Diff revision 5) Col: 80 E501 line too long (103 > 79 characters)
-
reviewboard/notifications/email.py (Diff revision 5) Col: 80 E501 line too long (120 > 79 characters)
-
-
-
reviewboard/notifications/email.py (Diff revision 5) We only want 2 blank lines between sections at the top level of a file.
-
reviewboard/notifications/email.py (Diff revision 5) Only one
#
for these.Make sure to wrap at <= 79 characters.
-
reviewboard/notifications/email.py (Diff revision 5) I suggest storing this as a constant at the top-level of the module.
Constants are also generally uppercase. This one should be more specific about being for this header. Maybe:
DIFF_FOR_HEADER_MAX_LENGTH
. -
-
reviewboard/notifications/email.py (Diff revision 5) So-called "magic numbers" (the 6) should always have a constant.
-
-
-
reviewboard/notifications/email.py (Diff revision 5) Make sure to test this. There's no
logger
.You also need to provide more information in the log message. As it is, if a server has thousands of users using it and keeps getting this, the admin will have no idea what review request or user this is being triggered for.
At a minimum, this should include the review request display ID (
review_request.display_id
). -
reviewboard/notifications/email.py (Diff revision 5) Not sure we want to put a header in here for this? I don't know whether
Warning
is a standard or not, but I'm not seeing it.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+30 -2) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py Tool: Pyflakes Processed Files: reviewboard/notifications/email.py
-
-
-
reviewboard/notifications/email.py (Diff revision 6) Can you document these? Refer to https://www.notion.so/reviewboard/Writing-Python-Docs-845c696271194d85b69503246db1d0e6#574a663ebae8453b8b014bd9ad841ce3
-
-
-
-
reviewboard/notifications/email.py (Diff revision 6) This should also explicitly say that this is when sending e-mail. Something like:
"Unable to store all filenames in X-ReviewBoard-Diff-For header when sending e-mail for review request %s: The header size exceeds the limit of %s. Remaining headers have been removed."
We also use format strings, rather than string concatenation. This is faster and allows for localization if we want it (we don't for logs, but if this were an exception or something sent to the UI, we would).
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+36 -2) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+36 -3) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py Tool: Pyflakes Processed Files: reviewboard/notifications/email.py
-
-
We're also going to need a unit test that tests overflowing this header and checking for warnings. One of the mentors there can guide you on this.
-
reviewboard/notifications/email.py (Diff revision 8) Almost there, but not quite right. This should be using the
#:
syntax as shown in my link, and there should be a comment per constant.Also, when you have literals like the "\r\n", you should wrap them in double backticks:
``\r\n``
-
reviewboard/notifications/email.py (Diff revision 8) I have a couple comments in this code block, but they're more to help learn about our code style. I have a bigger comment for this whole block, though.
Nuke it.
The reason is that we're not even done with headers by this stage. There's more that will happen further down, more in the mail client, etc. We have the limit set above as 8KB instead of something like 32KB because of the fact that there's going to be other headers put in in places.
So the maximum length and current length here is intended to be solely for this header. We're allowing 8KB for this header alone, leaving 24KB for the rest of the headers. Should be more than enough.
-
-
reviewboard/notifications/email.py (Diff revision 8) These should be the same statement. If you need to wrap lines, use parens, like:
current_header_length += (len(key) + len(value) + HEADER_ADDITIONAL_CHARACTERS_LENGTH)
-
reviewboard/notifications/email.py (Diff revision 8) I would actually reverse this. Try:
if current_header_length >= DIFF_FOR_HEADER_MAX_LENGTH: logging... break headers.appendlist(...)
-
reviewboard/notifications/email.py (Diff revision 8) Two important things here:
1) This isn't an error, it's a warning. We should use
logging.warning
.
2)"""
should never be used for anything but docstrings. This needs to use single quoted strings, formatted like:logging.warning('Unable to store all filenames ..... ' '................................... ' '................')
-
reviewboard/notifications/email.py (Diff revision 8) You don't need to (and shouldn't do)
str(...)
. The%s
will take care of it automatically.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+37 -4) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+37 -4) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+37 -4) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py
Summary: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||
Commit: |
|
|||||||||
Diff: |
Revision 12 (+76 -2) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py
-
reviewboard/notifications/email.py (Diff revision 12) Col: 68 E226 missing whitespace around arithmetic operator
-
-
-
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+76 -2) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py
-
reviewboard/notifications/tests.py (Diff revision 13) local variable 'from_email' is assigned to but never used
-
reviewboard/notifications/tests.py (Diff revision 13) local variable 'filediff' is assigned to but never used
-
-
reviewboard/notifications/email.py (Diff revision 13) Blank line between these two.
Can we also call the constant
MAX_FILENAME_HEADERS_LENGTH
? -
reviewboard/notifications/email.py (Diff revision 13) This should just be a regular string, not a raw string (no 'r' prefix). It should also use single quotes.
-
reviewboard/notifications/email.py (Diff revision 13) typos: "headrs", "addin". There's also an extra space at the beginning of this line.
-
reviewboard/notifications/email.py (Diff revision 13) extra "g" that got chopped off from the previous line.
-
reviewboard/notifications/email.py (Diff revision 13) This should wrap on word boundaries rather than breaking words:
logging.warning('Unable to store all filenames in ' 'X-ReviewBoard-Diff-For headers when ' 'sending e-mail for review request %s: ' ...
Make sure there's a space at the end of each of those lines so when they're concatenated, it still looks right.
-
reviewboard/notifications/email.py (Diff revision 13) logging.X()
will do a format operation, so these can be passed in as additional arguments rather than doing your own format. -
-
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+79 -2) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py
-
reviewboard/notifications/tests.py (Diff revision 14) local variable 'from_email' is assigned to but never used
-
reviewboard/notifications/tests.py (Diff revision 14) local variable 'filediff' is assigned to but never used
Summary: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 15 (+80 -3) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py
-
reviewboard/notifications/tests.py (Diff revision 15) Col: 33 E128 continuation line under-indented for visual indent
-
reviewboard/notifications/tests.py (Diff revision 15) Col: 33 E128 continuation line under-indented for visual indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 16 (+80 -3) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py
-
-
-
reviewboard/notifications/email.py (Diff revision 16) The contents of the docstring should be in the doc comments below. No sense splitting it up or repeating it.
-
-
reviewboard/notifications/email.py (Diff revision 16) This should not be a raw string. This will be a string that is four characters long:
>>> r'\r\n' '\\r\\n' >>> len(r'\r\n') 4
Also, single quotes.
-
-
reviewboard/notifications/email.py (Diff revision 16) This will take less lines and be easier to read as:
logging.warning( '.... ... ...' '... ... ...', ...)
-
-
reviewboard/notifications/tests.py (Diff revision 16) So we are going to generate a 100 character string 400 times instead of just doing it once. Instead we can generate the prefix once and then append the number in the loop:
prefix = 'x' * 1000 for i in range(400): filename = '%s%s' % (prefix, i)
It is also important to note that string concatenation (with
+
) has bad performance. It's not super bad, but if you're doing it 100 times it will take a long time. Using interpolation with%
is much faster in this case. It is better to use%
-formatting in general as we use it almost everywhere. -
-
reviewboard/notifications/tests.py (Diff revision 16) If we are always adding at least one character and we are truncating to the last 100, we can technically use
'X' * 99
. You may want to document this section to explain why you're truncating the file name. -
-
-
reviewboard/notifications/tests.py (Diff revision 16) You shouldn't need a
u'
prefix.We use
from __future__ import unicode_literals
at the start of each file, which turns'x'
intou'x'
behind the scenes. -
reviewboard/notifications/tests.py (Diff revision 16) spy.called
is equivalent tolen(spy.calls) > 0
.If
spy.called
isFalse
the above assertions will raise an exception. This should probably come before those assertions.
Summary: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||
Commit: |
|
||||||||||||
Diff: |
Revision 17 (+85 -3) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py
-
reviewboard/notifications/tests.py (Diff revision 17) local variable 'from_email' is assigned to but never used
-
-
-
-
-
-
-
reviewboard/notifications/email.py (Diff revision 17) Since this is a raw string, it will have length 4. Do not use a raw string (i.e., remove the
r
.).Also use single quotes.
-
reviewboard/notifications/tests.py (Diff revision 17) Undo this change. The line is unnecessary but should be fixed in a different patch.
-
reviewboard/notifications/tests.py (Diff revision 17) So
str
is not actually the string type for Python 2:unicode
is. However, using%
-formatting will call the relevant type conversion methods, e.g. when using%s
for anint
it will callunicode()
. (Or you could use%d
-- same difference.TL;DR: you can just do
(prefix, i)
-
reviewboard/notifications/tests.py (Diff revision 17) You don't need the backticks in comments because they will never be rendered.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 18 (+87 -3) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py
-
-
reviewboard/notifications/email.py (Diff revision 18) Col: 33 E128 continuation line under-indented for visual indent
-
reviewboard/notifications/email.py (Diff revision 18) Col: 33 E128 continuation line under-indented for visual indent
-
reviewboard/notifications/email.py (Diff revision 18) Col: 33 E128 continuation line under-indented for visual indent
-
reviewboard/notifications/tests.py (Diff revision 18) local variable 'from_email' is assigned to but never used
-
-
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 19 (+86 -1) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py
-
reviewboard/notifications/tests.py (Diff revision 19) local variable 'from_email' is assigned to but never used
-
Pending these nits, this looks good to me. Ship it!
-
reviewboard/notifications/email.py (Diff revision 19) This should just be a comment -- it needn't be a doc comment (
#:
). -
reviewboard/notifications/email.py (Diff revision 19) Can you surround X-Reviewboard-Diff-For with double backticks?
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 20 (+85 -1) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/notifications/tests.py
-
reviewboard/notifications/tests.py (Diff revision 20) local variable 'from_email' is assigned to but never used
-
-
reviewboard/notifications/email.py (Diff revision 20) This comment isn't entirely correct (we no longer have
DIFF_FOR_HEADER_MAX_LENGTH
), plus it's duplicating stuff on the individual doc comments for each variable. Let's just get rid of it. -
reviewboard/notifications/email.py (Diff revision 20) There's an extra set of parens around the log text that can be removed.
-
reviewboard/notifications/tests.py (Diff revision 20) Did you run this code? The
%s
s should have been filled in.
Branch: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 21 (+77) |
Checks run (2 succeeded, 1 failed with error)
-
-
-
-
reviewboard/notifications/tests.py (Diff revision 21) The ordering is a bit weird. I'd kinda suggest comparing the entirety of
args
at once.