Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py

 'formataddr' imported but unused

Col: 1
 W191 indentation contains tabs

Col: 1
 E101 indentation contains mixed spaces and tabs

Col: 1
 W191 indentation contains tabs

Col: 1
 W191 indentation contains tabs

Col: 1
 W191 indentation contains tabs

Col: 1
 W191 indentation contains tabs

Col: 1
 W191 indentation contains tabs

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

 undefined name 'counter'

 local variable 'counter' is assigned to but never used

Col: 1
 W191 indentation contains tabs

Col: 1
 W191 indentation contains tabs

Col: 1
 W191 indentation contains tabs

Col: 1
 W191 indentation contains tabs

Col: 1
 W191 indentation contains tabs

Col: 1
 W293 blank line contains whitespace

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py

Ignored Files:
    .project

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

Col: 29
 E701 multiple statements on one line (colon)

Col: 13
 E701 multiple statements on one line (colon)

Col: 9
 E122 continuation line missing indentation or outdented

Col: 1
 W293 blank line contains whitespace

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py

Ignored Files:
    .project

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

Col: 29
 E701 multiple statements on one line (colon)

Col: 13
 E701 multiple statements on one line (colon)

Col: 9
 E122 continuation line missing indentation or outdented

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py

Ignored Files:
    .project



Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py

Ignored Files:
    .project

 'formataddr' imported but unused

Does this have a bug number?

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.

It looks like your editor created this. Please remove it from your change.

Please put a blank line between these.

Please add a comment explaining why we do this complex stuff.

We should iterate this as for key, value in six.iteritems(headers):

These lines could all be combined.

Please put a blank line between these.

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?)

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.

Please put a blank line between these.

It might be nice to pull this out into a constant.

This could be current_header_length = newlength, since we already did that same computation.

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.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py

Col: 1
 W293 blank line contains whitespace

Col: 1
 W293 blank line contains whitespace

Col: 5
 E266 too many leading '#' for block comment

Col: 5
 E303 too many blank lines (3)

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

Col: 5
 E266 too many leading '#' for block comment

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

Col: 1
 W293 blank line contains whitespace

Col: 1
 W293 blank line contains whitespace

Col: 9
 E266 too many leading '#' for block comment

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

Col: 9
 E266 too many leading '#' for block comment

Col: 1
 W293 blank line contains whitespace

Col: 1
 W293 blank line contains whitespace

Col: 5
 E303 too many blank lines (2)

Col: 1
 W293 blank line contains whitespace

Col: 1
 W293 blank line contains whitespace

Col: 9
 E122 continuation line missing indentation or outdented

Col: 56
 E701 multiple statements on one line (colon)

Col: 1
 W293 blank line contains whitespace

Col: 1
 W293 blank line contains whitespace

Col: 9
 E122 continuation line missing indentation or outdented

Col: 13
 E701 multiple statements on one line (colon)

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

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

Col: 1
 W293 blank line contains whitespace

We only want 2 blank lines between sections at the top level of a file.

Only one # for these.
Make sure to wrap at <= 79 characters.

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.

\r\n, not /r/n.

So-called "magic numbers" (the 6) should always have a constant.

No blank line here.

No blank line.

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).

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.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py



Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py

 'formataddr' imported but unused

Can you document these? Refer to https://www.notion.so/reviewboard/Writing-Python-Docs-845c696271194d85b69503246db1d0e6#574a663ebae8453b8b014bd9ad841ce3

Comments should wrap at around character 79.

No blank line here.

No parens needed.

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).

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py

 'formataddr' imported but unused

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.

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``

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.

You can fit more per line.

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)

I would actually reverse this. Try:

if current_header_length >= DIFF_FOR_HEADER_MAX_LENGTH:
    logging...
    break


headers.appendlist(...)

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 ..... '
                '................................... '
                '................')

You don't need to (and shouldn't do) str(...). The %s will take care of it automatically.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py

Col: 72
 W291 trailing whitespace

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py

Col: 68
 E226 missing whitespace around arithmetic operator

Col: 1
 W293 blank line contains whitespace

Col: 70
 W291 trailing whitespace

Col: 1
 W293 blank line contains whitespace

Col: 1
 W293 blank line contains whitespace

Col: 1
 W293 blank line contains whitespace

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

 local variable 'from_email' is assigned to but never used

 local variable 'filediff' is assigned to but never used

Blank line between these two.
Can we also call the constant MAX_FILENAME_HEADERS_LENGTH?

This should just be a regular string, not a raw string (no 'r' prefix). It should also use single quotes.

typos: "headrs", "addin". There's also an extra space at the beginning of this line.

extra "g" that got chopped off from the previous line.

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.

logging.X() will do a format operation, so these can be passed in as additional arguments rather than doing your own format.

Add a blank line between these two.

Please use single-quotes around 'X'

Leftover debug code?

Please wrap the string on word boundaries.

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

 local variable 'from_email' is assigned to but never used

 local variable 'filediff' is assigned to but never used

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

Col: 33
 E128 continuation line under-indented for visual indent

Col: 33
 E128 continuation line under-indented for visual indent

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

Your summary should mention that it is an e-mail header because HTTP also has headers.

The contents of the docstring should be in the doc comments below. No sense splitting it up or repeating it.

Looks like it got a bit mangled at the end.

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.

Colon instead of semicolon.

This will take less lines and be easier to read as:

logging.warning(
    '.... ... ...'
    '... ... ...',
    ...)

Undo this change.

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.

Can we just rename this filename?

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.

"and" should not be split over two lines.

Colon instead of semicolon here.

You shouldn't need a u' prefix.
We use from __future__ import unicode_literals at the start of each file, which turns 'x' into u'x' behind the scenes.