Added cap to filename header

Review Request #8646 — Created Jan. 20, 2017 and submitted

Information

Review Board
master
7a9e403...

Reviewers

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?

daviddavid

Please flesh out your description to include more info on what the bug was and how you fixed it. See …

daviddavid

We're also going to need a unit test that tests overflowing this header and checking for warnings. One of the …

chipx86chipx86

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

brenniebrennie

'formataddr' imported but unused

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

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

reviewbotreviewbot

undefined name 'counter'

reviewbotreviewbot

local variable 'counter' is assigned to but never used

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E122 continuation line missing indentation or outdented

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E122 continuation line missing indentation or outdented

reviewbotreviewbot

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

daviddavid

'formataddr' imported but unused

reviewbotreviewbot

Please put a blank line between these.

daviddavid

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

daviddavid

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

daviddavid

These lines could all be combined.

daviddavid

Please put a blank line between these.

daviddavid

We should have a comment explaining what this "2" is for. We should also include some extra padding in here …

daviddavid

It might be nice to pull out 'X-ReviewBoard-Diff-For' into a constant string. We can also compute the length of it …

daviddavid

Please put a blank line between these.

daviddavid

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

daviddavid

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

daviddavid

After a block, please put a newline. Can we add a log statement to the break case to record that …

daviddavid

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

chipx86chipx86

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 5 E303 too many blank lines (3)

reviewbotreviewbot

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

reviewbotreviewbot

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

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

I suggest storing this as a constant at the top-level of the module. Constants are also generally uppercase. This one …

chipx86chipx86

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

\r\n, not /r/n.

chipx86chipx86

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

reviewbotreviewbot

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

chipx86chipx86

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 9 E122 continuation line missing indentation or outdented

reviewbotreviewbot

No blank line here.

chipx86chipx86

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

No blank line.

chipx86chipx86

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 9 E122 continuation line missing indentation or outdented

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Make sure to test this. There's no logger. You also need to provide more information in the log message. As …

chipx86chipx86

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

reviewbotreviewbot

Not sure we want to put a header in here for this? I don't know whether Warning is a standard …

chipx86chipx86

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

'formataddr' imported but unused

reviewbotreviewbot

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

chipx86chipx86

Comments should wrap at around character 79.

chipx86chipx86

No blank line here.

chipx86chipx86

No parens needed.

chipx86chipx86

This should also explicitly say that this is when sending e-mail. Something like: "Unable to store all filenames in X-ReviewBoard-Diff-For …

chipx86chipx86

'formataddr' imported but unused

reviewbotreviewbot

Almost there, but not quite right. This should be using the #: syntax as shown in my link, and there …

chipx86chipx86

I have a couple comments in this code block, but they're more to help learn about our code style. I …

chipx86chipx86

You can fit more per line.

chipx86chipx86

These should be the same statement. If you need to wrap lines, use parens, like: current_header_length += (len(key) + len(value) …

chipx86chipx86

I would actually reverse this. Try: if current_header_length >= DIFF_FOR_HEADER_MAX_LENGTH: logging... break headers.appendlist(...)

chipx86chipx86

Two important things here: 1) This isn't an error, it's a warning. We should use logging.warning. 2) """ should never …

chipx86chipx86

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

chipx86chipx86

Col: 72 W291 trailing whitespace

reviewbotreviewbot

Col: 68 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 70 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

This should wrap on word boundaries rather than breaking words: logging.warning('Unable to store all filenames in ' 'X-ReviewBoard-Diff-For headers when …

daviddavid

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

daviddavid

Add a blank line between these two.

daviddavid

local variable 'from_email' is assigned to but never used

reviewbotreviewbot

Please use single-quotes around 'X'

daviddavid

local variable 'filediff' is assigned to but never used

reviewbotreviewbot

Leftover debug code?

daviddavid

Please wrap the string on word boundaries.

daviddavid

local variable 'from_email' is assigned to but never used

reviewbotreviewbot

local variable 'filediff' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

brenniebrennie

Looks like it got a bit mangled at the end.

brenniebrennie

This should not be a raw string. This will be a string that is four characters long: >>> r'\r\n' '\\r\\n' …

brenniebrennie

Colon instead of semicolon.

brenniebrennie

This will take less lines and be easier to read as: logging.warning( '.... ... ...' '... ... ...', ...)

brenniebrennie

Undo this change.

brenniebrennie

So we are going to generate a 100 character string 400 times instead of just doing it once. Instead we …

brenniebrennie

Can we just rename this filename?

brenniebrennie

If we are always adding at least one character and we are truncating to the last 100, we can technically …

brenniebrennie

"and" should not be split over two lines.

brenniebrennie

Colon instead of semicolon here.

brenniebrennie

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

brenniebrennie

spy.called is equivalent to len(spy.calls) > 0. If spy.called is False the above assertions will raise an exception. This should …

brenniebrennie

Since this is a raw string, it will have length 4. Do not use a raw string (i.e., remove the …

brenniebrennie

Undo this change. The line is unnecessary but should be fixed in a different patch.

brenniebrennie

local variable 'from_email' is assigned to but never used

reviewbotreviewbot

Col: 26 E703 statement ends with a semicolon

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

So str is not actually the string type for Python 2: unicode is. However, using %-formatting will call the relevant …

brenniebrennie

Col: 49 W291 trailing whitespace

reviewbotreviewbot

Col: 70 W291 trailing whitespace

reviewbotreviewbot

Col: 77 W291 trailing whitespace

reviewbotreviewbot

You don't need the backticks in comments because they will never be rendered.

brenniebrennie

'formataddr' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

local variable 'from_email' is assigned to but never used

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 49 W291 trailing whitespace

reviewbotreviewbot

Col: 70 W291 trailing whitespace

reviewbotreviewbot

Col: 77 W291 trailing whitespace

reviewbotreviewbot

This should just be a comment -- it needn't be a doc comment (#:).

brenniebrennie

Can you surround X-Reviewboard-Diff-For with double backticks?

brenniebrennie

Likewise here?

brenniebrennie

And the whole header thing here too.

brenniebrennie

local variable 'from_email' is assigned to but never used

reviewbotreviewbot

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 …

daviddavid

There's an extra set of parens around the log text that can be removed.

daviddavid

local variable 'from_email' is assigned to but never used

reviewbotreviewbot

Did you run this code? The %ss should have been filled in.

daviddavid

You can fit more per line.

chipx86chipx86

Blank line before comments.

chipx86chipx86

The ordering is a bit weird. I'd kinda suggest comparing the entirety of args at once.

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    
  2. reviewboard/notifications/email.py (Diff revision 1)
     
     
     'formataddr' imported but unused
    
    1. This issue was in the original email.py; should I remove the import?

  3. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  4. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     E101 indentation contains mixed spaces and tabs
    
  5. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  6. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  7. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  8. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  9. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  10. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  11. reviewboard/notifications/email.py (Diff revision 1)
     
     
     undefined name 'counter'
    
  12. reviewboard/notifications/email.py (Diff revision 1)
     
     
     local variable 'counter' is assigned to but never used
    
  13. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  14. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  15. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  16. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  17. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  18. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  19. 
      
DA
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        .project
    
    
  2. reviewboard/notifications/email.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  3. reviewboard/notifications/email.py (Diff revision 2)
     
     
    Col: 29
     E701 multiple statements on one line (colon)
    
  4. reviewboard/notifications/email.py (Diff revision 2)
     
     
    Col: 13
     E701 multiple statements on one line (colon)
    
  5. reviewboard/notifications/email.py (Diff revision 2)
     
     
    Col: 9
     E122 continuation line missing indentation or outdented
    
  6. reviewboard/notifications/email.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  7. 
      
DA
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        .project
    
    
  2. reviewboard/notifications/email.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  3. reviewboard/notifications/email.py (Diff revision 3)
     
     
    Col: 29
     E701 multiple statements on one line (colon)
    
  4. reviewboard/notifications/email.py (Diff revision 3)
     
     
    Col: 13
     E701 multiple statements on one line (colon)
    
  5. reviewboard/notifications/email.py (Diff revision 3)
     
     
    Col: 9
     E122 continuation line missing indentation or outdented
    
  6. 
      
DA
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        .project
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        .project
    
    
  2. reviewboard/notifications/email.py (Diff revision 4)
     
     
     'formataddr' imported but unused
    
  3. 
      
david
  1. 
      
  2. Does this have a bug number?

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

  4. .project (Diff revision 4)
     
     

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

  5. reviewboard/notifications/email.py (Diff revision 4)
     
     
     

    Please put a blank line between these.

  6. reviewboard/notifications/email.py (Diff revision 4)
     
     

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

  7. reviewboard/notifications/email.py (Diff revision 4)
     
     

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

  8. reviewboard/notifications/email.py (Diff revision 4)
     
     
     
     

    These lines could all be combined.

  9. reviewboard/notifications/email.py (Diff revision 4)
     
     
     

    Please put a blank line between these.

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

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

  12. reviewboard/notifications/email.py (Diff revision 4)
     
     
     

    Please put a blank line between these.

  13. reviewboard/notifications/email.py (Diff revision 4)
     
     

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

  14. reviewboard/notifications/email.py (Diff revision 4)
     
     

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

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

  16. 
      
DA
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    
  2. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 5
     E266 too many leading '#' for block comment
    
  5. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 5
     E303 too many blank lines (3)
    
  6. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (123 > 79 characters)
    
  7. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 5
     E266 too many leading '#' for block comment
    
  8. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (115 > 79 characters)
    
  9. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  10. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  11. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 9
     E266 too many leading '#' for block comment
    
  12. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (106 > 79 characters)
    
  13. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 9
     E266 too many leading '#' for block comment
    
  14. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  15. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  16. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  17. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  18. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  19. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 9
     E122 continuation line missing indentation or outdented
    
  20. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 56
     E701 multiple statements on one line (colon)
    
  21. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  22. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  23. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 9
     E122 continuation line missing indentation or outdented
    
  24. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 13
     E701 multiple statements on one line (colon)
    
  25. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (103 > 79 characters)
    
  26. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (120 > 79 characters)
    
  27. reviewboard/notifications/email.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  28. 
      
chipx86
  1. 
      
  2. reviewboard/notifications/email.py (Diff revision 5)
     
     
     
     
     
     

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

  3. reviewboard/notifications/email.py (Diff revision 5)
     
     
     

    Only one # for these.

    Make sure to wrap at <= 79 characters.

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

  5. reviewboard/notifications/email.py (Diff revision 5)
     
     

    \r\n, not /r/n.

  6. reviewboard/notifications/email.py (Diff revision 5)
     
     

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

  7. reviewboard/notifications/email.py (Diff revision 5)
     
     
     
     

    No blank line here.

  8. reviewboard/notifications/email.py (Diff revision 5)
     
     
     
     

    No blank line.

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

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

  11. 
      
DA
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    
  2. reviewboard/notifications/email.py (Diff revision 6)
     
     
     'formataddr' imported but unused
    
    1. If this is showing up, it's worth fixing.

      I don't have this in my tree, though?

  3. 
      
chipx86
  1. 
      
  2. reviewboard/notifications/email.py (Diff revision 6)
     
     
     

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

  3. reviewboard/notifications/email.py (Diff revision 6)
     
     
     
     
     
     

    Comments should wrap at around character 79.

  4. reviewboard/notifications/email.py (Diff revision 6)
     
     
     
     

    No blank line here.

  5. reviewboard/notifications/email.py (Diff revision 6)
     
     
     

    No parens needed.

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

  7. 
      
brennie
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    
  2. reviewboard/notifications/email.py (Diff revision 7)
     
     
     'formataddr' imported but unused
    
  3. 
      
DA
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    
  2. 
      
chipx86
  1. 
      
  2. 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.

  3. 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``
    
  4. 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.

  5. reviewboard/notifications/email.py (Diff revision 8)
     
     
     
     

    You can fit more per line.

  6. 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)
    
  7. 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(...)
    
  8. 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 ..... '
                    '................................... '
                    '................')
    
  9. 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.

  10. 
      
DA
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    
  2. 
      
DA
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    
  2. reviewboard/notifications/email.py (Diff revision 10)
     
     
    Col: 72
     W291 trailing whitespace
    
  3. 
      
DA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    
  2. 
      
DA
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
        reviewboard/notifications/tests.py
    
    
  2. reviewboard/notifications/email.py (Diff revision 12)
     
     
    Col: 68
     E226 missing whitespace around arithmetic operator
    
  3. reviewboard/notifications/tests.py (Diff revision 12)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. reviewboard/notifications/tests.py (Diff revision 12)
     
     
    Col: 70
     W291 trailing whitespace
    
  5. reviewboard/notifications/tests.py (Diff revision 12)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/notifications/tests.py (Diff revision 12)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  7. reviewboard/notifications/tests.py (Diff revision 12)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  8. 
      
DA
DA
reviewbot
  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
    
    
  2. reviewboard/notifications/tests.py (Diff revision 13)
     
     
     local variable 'from_email' is assigned to but never used
    
  3. reviewboard/notifications/tests.py (Diff revision 13)
     
     
     local variable 'filediff' is assigned to but never used
    
  4. 
      
DA
david
  1. 
      
  2. reviewboard/notifications/email.py (Diff revision 13)
     
     
     

    Blank line between these two.

    Can we also call the constant MAX_FILENAME_HEADERS_LENGTH?

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

    1. Should this be a regular string?

      The e-mail header will be containing \r\n, which means 4 bytes and not 2. len('\r\n') resolves to 2, while len(r'\r\n') resolves to 4 which seems like the intended behavior.

    2. I don't see explicit \r\n (with backslashes) when I look at the raw message headers from a delivered e-mail. Where do you see that?

  4. reviewboard/notifications/email.py (Diff revision 13)
     
     

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

  5. reviewboard/notifications/email.py (Diff revision 13)
     
     

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

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

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

  8. reviewboard/notifications/email.py (Diff revision 13)
     
     
     

    Add a blank line between these two.

  9. reviewboard/notifications/tests.py (Diff revision 13)
     
     

    Please use single-quotes around 'X'

  10. reviewboard/notifications/tests.py (Diff revision 13)
     
     
     

    Leftover debug code?

  11. reviewboard/notifications/tests.py (Diff revision 13)
     
     
     
     
     
     

    Please wrap the string on word boundaries.

  12. 
      
DA
reviewbot
  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
    
    
  2. reviewboard/notifications/tests.py (Diff revision 14)
     
     
     local variable 'from_email' is assigned to but never used
    
  3. reviewboard/notifications/tests.py (Diff revision 14)
     
     
     local variable 'filediff' is assigned to but never used
    
  4. 
      
DA
DA
reviewbot
  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
    
    
  2. reviewboard/notifications/tests.py (Diff revision 15)
     
     
    Col: 33
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/notifications/tests.py (Diff revision 15)
     
     
    Col: 33
     E128 continuation line under-indented for visual indent
    
  4. 
      
DA
reviewbot
  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
    
    
  2. 
      
brennie
  1. 
      
  2. Your summary should mention that it is an e-mail header because HTTP also has headers.

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

  4. reviewboard/notifications/email.py (Diff revision 16)
     
     

    Looks like it got a bit mangled at the end.

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

  6. reviewboard/notifications/email.py (Diff revision 16)
     
     

    Colon instead of semicolon.

  7. reviewboard/notifications/email.py (Diff revision 16)
     
     
     
     
     
     
     
     

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

    logging.warning(
        '.... ... ...'
        '... ... ...',
        ...)
    
  8. reviewboard/notifications/tests.py (Diff revision 16)
     
     

    Undo this change.

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

  10. reviewboard/notifications/tests.py (Diff revision 16)
     
     

    Can we just rename this filename?

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

  12. reviewboard/notifications/tests.py (Diff revision 16)
     
     
     

    "and" should not be split over two lines.

  13. reviewboard/notifications/tests.py (Diff revision 16)
     
     

    Colon instead of semicolon here.

  14. 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' into u'x' behind the scenes.

  15. reviewboard/notifications/tests.py (Diff revision 16)
     
     

    spy.called is equivalent to len(spy.calls) > 0.

    If spy.called is False the above assertions will raise an exception. This should probably come before those assertions.

  16. 
      
DA
reviewbot
  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
    
    
  2. reviewboard/notifications/tests.py (Diff revision 17)
     
     
     local variable 'from_email' is assigned to but never used
    
  3. reviewboard/notifications/tests.py (Diff revision 17)
     
     
    Col: 26
     E703 statement ends with a semicolon
    
  4. reviewboard/notifications/tests.py (Diff revision 17)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  5. reviewboard/notifications/tests.py (Diff revision 17)
     
     
    Col: 49
     W291 trailing whitespace
    
  6. reviewboard/notifications/tests.py (Diff revision 17)
     
     
    Col: 70
     W291 trailing whitespace
    
  7. reviewboard/notifications/tests.py (Diff revision 17)
     
     
    Col: 77
     W291 trailing whitespace
    
  8. 
      
brennie
  1. 
      
  2. 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.

    1. Shouldn't this be a raw string? It seems like the length should be 4, right? I think I have previously responded to an issue on this subject.

    2. From RFC 2822

      Messages are divided into lines of characters. A line is a series of
      characters that is delimited with the two characters carriage-return
      and line-feed; that is, the carriage return (CR) character (ASCII
      value 13) followed immediately by the line feed (LF) character (ASCII
      value 10). (The carriage-return/line-feed pair is usually written in
      this document as "CRLF".)

  3. reviewboard/notifications/tests.py (Diff revision 17)
     
     

    Undo this change. The line is unnecessary but should be fixed in a different patch.

  4. 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 an int it will call unicode(). (Or you could use %d -- same difference.

    TL;DR: you can just do (prefix, i)

  5. reviewboard/notifications/tests.py (Diff revision 17)
     
     

    You don't need the backticks in comments because they will never be rendered.

  6. 
      
DA
reviewbot
  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
    
    
  2. reviewboard/notifications/email.py (Diff revision 18)
     
     
     'formataddr' imported but unused
    
  3. reviewboard/notifications/email.py (Diff revision 18)
     
     
    Col: 33
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/notifications/email.py (Diff revision 18)
     
     
    Col: 33
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/notifications/email.py (Diff revision 18)
     
     
    Col: 33
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/notifications/tests.py (Diff revision 18)
     
     
     local variable 'from_email' is assigned to but never used
    
  7. reviewboard/notifications/tests.py (Diff revision 18)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  8. reviewboard/notifications/tests.py (Diff revision 18)
     
     
    Col: 49
     W291 trailing whitespace
    
  9. reviewboard/notifications/tests.py (Diff revision 18)
     
     
    Col: 70
     W291 trailing whitespace
    
  10. reviewboard/notifications/tests.py (Diff revision 18)
     
     
    Col: 77
     W291 trailing whitespace
    
  11. 
      
DA
reviewbot
  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
    
    
  2. reviewboard/notifications/tests.py (Diff revision 19)
     
     
     local variable 'from_email' is assigned to but never used
    
  3. 
      
DA
brennie
  1. Pending these nits, this looks good to me. Ship it!

  2. reviewboard/notifications/email.py (Diff revision 19)
     
     
     
     
     
     
     

    This should just be a comment -- it needn't be a doc comment (#:).

  3. reviewboard/notifications/email.py (Diff revision 19)
     
     

    Can you surround X-Reviewboard-Diff-For with double backticks?

  4. reviewboard/notifications/email.py (Diff revision 19)
     
     

    Likewise here?

  5. reviewboard/notifications/email.py (Diff revision 19)
     
     
     

    And the whole header thing here too.

  6. 
      
DA
reviewbot
  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
    
    
  2. reviewboard/notifications/tests.py (Diff revision 20)
     
     
     local variable 'from_email' is assigned to but never used
    
  3. 
      
DA
  1. Ship It!
  2. 
      
DA
  1. Ship It!
  2. 
      
david
  1. 
      
  2. 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.

  3. reviewboard/notifications/email.py (Diff revision 20)
     
     
     
     
     
     
     

    There's an extra set of parens around the log text that can be removed.

  4. reviewboard/notifications/tests.py (Diff revision 20)
     
     
     
     
     
     

    Did you run this code? The %ss should have been filled in.

    1. I think my code is correct. The test returns true. I believe that logging does the interpolation itself, so args[0] is the string before %-interpolation.

    2. I test for args[1] and args[2] in the assertions directly above it. Would it be better to test it all in one assertion? How would I call the spy to get the post-interpolation string?

  5. 
      
DA
chipx86
  1. 
      
  2. reviewboard/notifications/email.py (Diff revision 21)
     
     
     
     
     
     

    You can fit more per line.

  3. reviewboard/notifications/tests.py (Diff revision 21)
     
     
     

    Blank line before comments.

  4. reviewboard/notifications/tests.py (Diff revision 21)
     
     
     
     

    The ordering is a bit weird. I'd kinda suggest comparing the entirety of args at once.

  5. 
      
david
  1. Making a few style/cleanup changes and landing. Thanks!

  2. 
      
DA
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (5e19619)
Loading...