Diff Doctor

Review Request #6993 — Created Feb. 27, 2015 and discarded

Information

Review Board
master

Reviewers

This project attempts to provide a more helpful user interface when a patch fails.
A patch fails when either components: the patch file, or the to-be-patched file is corrupt. Less likely, linux patch can also fail.
The first step of this project is to catch the error in patch() as a PatchError, and provide the user a download link to the reject files as a ZIP file. This link will be implemented as a Django view.
The second step of this project involves displaying the reject files (original, diff and .rej files), rendered as in-line HTML with Pygments, in place of the error traceback.

Manual testing has been done, for:
- Normal review requests (no diff error), returns a 404
- Review request containing a failed patch, with failure due to faulty original-file (corrupted endpoints in 3rd party services with their own repositories, like gitweb, or cgit). These will return a 404 page
- Review request containing a failed patch, with failure due to patch() itself. I have only managed to replicate this by manually uploading a corrupted diff file, but am unaware of how this could happen in a real use case. The in-line reject files show up as expected in the traceback box, and the .zip download is functional

Description From Last Updated

redefinition of unused 'get_object_or_404' from line 18

reviewbotreviewbot

redefinition of unused 'DiffSet' from line 49

reviewbotreviewbot

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

reviewbotreviewbot

redefinition of unused 'get_patched_file' from line 43

reviewbotreviewbot

redefinition of unused 'get_original_file' from line 43

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 43 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 22 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 34 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 25 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 28 E231 missing whitespace after ':'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 34 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 25 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 28 E231 missing whitespace after ':'

reviewbotreviewbot

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

reviewbotreviewbot

redefinition of unused 'get_object_or_404' from line 18

reviewbotreviewbot

redefinition of unused 'DiffSet' from line 49

reviewbotreviewbot

redefinition of unused 'get_patched_file' from line 43

reviewbotreviewbot

redefinition of unused 'get_original_file' from line 43

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 22 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 43 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 44 W292 no newline at end of file

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

redefinition of unused 'get_object_or_404' from line 18

reviewbotreviewbot

redefinition of unused 'DiffSet' from line 49

reviewbotreviewbot

redefinition of unused 'get_original_file' from line 43

reviewbotreviewbot

redefinition of unused 'get_patched_file' from line 43

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

local variable 'new_file' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 17 E225 missing whitespace around operator

reviewbotreviewbot

Col: 38 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 29 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 32 E231 missing whitespace after ':'

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

redefinition of unused 'DiffSet' from line 49

reviewbotreviewbot

redefinition of unused 'get_patched_file' from line 43

reviewbotreviewbot

redefinition of unused 'get_original_file' from line 43

reviewbotreviewbot

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

reviewbotreviewbot

redefinition of unused 'DiffSet' from line 49

reviewbotreviewbot

redefinition of unused 'get_original_file' from line 43

reviewbotreviewbot

redefinition of unused 'get_patched_file' from line 43

reviewbotreviewbot

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

reviewbotreviewbot

redefinition of unused 'DiffSet' from line 49

reviewbotreviewbot

redefinition of unused 'get_original_file' from line 43

reviewbotreviewbot

redefinition of unused 'get_patched_file' from line 43

reviewbotreviewbot

Undo this change.

brenniebrennie

Use all available line width (79 characters).

brenniebrennie

We use single quotes. Also, if you wrap the string in parenthesis you can leave out the backslashes (which are …

brenniebrennie

This should all be lined up with the opening quote on the previous lines.

brenniebrennie

You want to do: super(PatchError, self).__init__(error_msg) and it is usually done at the start of the overriden __init__.

brenniebrennie

Who did what can be shown with git blame so this isn't really needed.

brenniebrennie

This method needs a docstring.

brenniebrennie

This seems like good information for the docstring.

brenniebrennie

You should use from django.utils.six import StringIO. It would probably be better to use cStringIO à la from django.utils.six.moves import …

brenniebrennie

Order these alphabetically.

brenniebrennie

Order these alphabetically.

brenniebrennie

This class needs a docstring.

brenniebrennie

This method needs a docstring.

brenniebrennie

You can format this as: self.review_request, response = _find_review_request( request, review_request_id, None)

brenniebrennie

Why does this assign to self if there are no other methods in that will use it?

brenniebrennie

Does this take into account local sites?

brenniebrennie

Comments generally are written in second person (We...) or like follows: There shouldn't be more than one diff returned. Also, …

brenniebrennie

Since the error handling is the same you can do except (FileNotFoundError, SCMError) as e: # common error handling

brenniebrennie

You don't implement render_to_response so why are you explicitly invoking the super method? Also, this formatting is wonky. How about: …

brenniebrennie

Same as previous comment.

brenniebrennie

We use single quotes for strings. Also this will have to be localized.

brenniebrennie

It would probably be better to use a StreamingHttpResponse here so we don't block while the zip is built.

brenniebrennie

I mentioned in my last review that since you are not using overriding render_to_response, you should just use self.render_to_response. Since …

brenniebrennie

Single quotes for strings. Should have a trailing comma.

brenniebrennie

Single quotes for strings. Should have a trailing comma.

brenniebrennie

Single quotes for strings. Should have a trailing comma. The %-substitution should happen outside the call to _. Otherwise, multiple …

brenniebrennie

We use single quotes for strings.

brenniebrennie

This method needs a docstring.

brenniebrennie

This information seems like it would be good for the docstring instead of as just a comment.

brenniebrennie

No blank line here.

brenniebrennie

The braces should be on their own lines. Each key-value pair should be its own line. The last key-value pair …

brenniebrennie

We use single quotes for strings.

brenniebrennie

Strings need to be translated. Here and elsewhere. I also don't think this is a good title -- perhaps just …

brenniebrennie

Why not just {% trans "There was a problem with your patch." %}. Also, missing a period.

brenniebrennie

You can just use {% trans error.msg %}

brenniebrennie

This doesn't seem like a good string to put here. Also, make sure all strings are localized.

brenniebrennie

is {{error}} expected to be HTML? If not, just use {% trans error %}.

brenniebrennie

Col: 13 E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

Col: 13 E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

'HttpResponseServerError' imported but unused

reviewbotreviewbot

'render' imported but unused

reviewbotreviewbot

'FileNotFoundError' imported but unused

reviewbotreviewbot

'SCMError' imported but unused

reviewbotreviewbot

Imports should be in alphabetical order.

chipx86chipx86

Can you add a docstring for this?

chipx86chipx86

Considering we'll only be accessing this data if we're downloading it, it's a bit expensive to do every time we're …

chipx86chipx86

Comments should be in sentence casing and end with a period.

chipx86chipx86

No need for mode=. You can just pass 'r'.

chipx86chipx86

We should localize this.

chipx86chipx86

Blank line before this.

chipx86chipx86

This change isn't relevant, and should be undone.

chipx86chipx86

Missing a trailing period.

chipx86chipx86

This needs to follow the guidelines for docstrings we have elsewhere. No :param: or :return:, either.

chipx86chipx86

Must be in alphabetical order.

chipx86chipx86

Should use dashes instead of underscores in the URL. This is also missing a $ at the end of the …

chipx86chipx86

We use dashes instead of underscores for URL names.

chipx86chipx86

The text must be on the first line. Instead of "This class ...", say something like "Downloads debug information for …

chipx86chipx86

This information is already in the function, and is more of an implementation detail for the URL structure. I would …

chipx86chipx86

Make sure to follow the proper docstring format: """One-line summary Multi-line description. """

chipx86chipx86

Your code isn't actually streaming the content, so there's no reason to use a StreamingHttpResponse. A StreamingHttpResponse would be useful …

chipx86chipx86

There's not much benefit to using a format string here, if we're hard-coding the string to put in there.

chipx86chipx86

If there's a different sort of exception, we need to know about it. This should definitely remain a 500. It's …

chipx86chipx86

This needs a docstring.

chipx86chipx86

You can write this as one statement: return local_site_reverse( 'patch-download', request=self.request, kwargs={ ... })

chipx86chipx86

That's not adding anything beyond the name of the function. The docstring should explain what information is being added.

chipx86chipx86

Needs to be proper sentences.

chipx86chipx86

This would be better as: error_data = ( (e.file, 'old_file'), ... ) for code, context_attr in error_data: ...

chipx86chipx86

Make sure the indentation of all these template tags are correct, with respect to any parent template tags.

chipx86chipx86

#oldfile is going to conflict with other errors on the page. Each ID needs to be unique. Note how we …

chipx86chipx86

It's best to call mark_safe() around the data being fed into the template, when we know it's safe.

chipx86chipx86

These lines are way too short. Also, no spaces inside the {{...}}.

chipx86chipx86

'HttpResponseServerError' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

'HttpResponseServerError' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
     redefinition of unused 'get_object_or_404' from line 18
    
  3. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
     redefinition of unused 'DiffSet' from line 49
    
  4. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (96 > 79 characters)
    
  5. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
     redefinition of unused 'get_patched_file' from line 43
    
  6. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
     redefinition of unused 'get_original_file' from line 43
    
  7. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (100 > 79 characters)
    
  8. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  9. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  10. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 43
     E231 missing whitespace after ':'
    
  11. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 22
     E231 missing whitespace after ':'
    
  12. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  13. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 35
     E128 continuation line under-indented for visual indent
    
  14. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (94 > 79 characters)
    
  15. 
      
TI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
  2. reviewboard/diffviewer/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 34
     E231 missing whitespace after ':'
    
  3. reviewboard/diffviewer/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 25
     E231 missing whitespace after ':'
    
  4. reviewboard/diffviewer/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 28
     E231 missing whitespace after ':'
    
  5. reviewboard/diffviewer/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  6. 
      
chipx86
  1. Thanks for getting the draft of this change up. Make sure you prefix the summary with "[WIP]".

    It looks like this is still in the very early phases. I have a few comments to help with the next steps:

    1. The files in reviewboard/diffviewer/ know nothing about review requests, and that must not change. You won't want to generate the URLs in there, but rather in the templates, which do have access to the review request. You'll use the {% url %} tag there.
    2. There's not really any information in this change as to why your custom view did not work. Can you let me know what issues you hit?
    3. In our codebase, the diffviewer module is meant to be consumed by the reviews module. The URLs will all live in reviews/urls.py, and map to views in reviews that consume views or other code from diffviewer. You'll need to follow the same pattern. Links in the template would map to URLs/views in reviews, which would have access to the review request (same as any other views in there). Those views would then make use of code from diffviewer to fetch the files from the result of a patch() call, and return them.

    What I want to see in the next revision is some work toward this. Instead of testing code with your name in it, try to work toward a working implementation that, for now, just returns a dummy file (something generated in code). You should have:

    1. A URL in reviews for this. I suggest putting this in the dowload_diff_urls section as '^patch-results/$'.
    2. A view in reviews for downloading the debug information for a diff. This would be what the URL maps to. This would get the matching review request and, for now, return some dummy content. Look at the other views mapped from those neighboring URLs.
    3. A link in the template for "Download patch results" pointing to that URL

    This will give you something that should be testable. You should be able to click that link and get that dummy content as something that's downloadable.

    The next step is to create a new exception in diffviewer/errors.py called PatchFailedError. This should take the same arguments currently fed into the exception in patch(), and should generate that string by itself. Those arguments should also be accessible to any code that catches the exception. They're just variables on the instance of the exception at that point.

    After that, it's time to modify your new view to call patch(), catch the exception, and then create an archive of the contents of that temp directory mentioned in the exception. Python has modules built-in for creating archives.

    This would comprise the introductory step to this project.

  2. 
      
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
  2. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 34
     E231 missing whitespace after ':'
    
  3. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 25
     E231 missing whitespace after ':'
    
  4. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 28
     E231 missing whitespace after ':'
    
  5. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
     redefinition of unused 'get_object_or_404' from line 18
    
  7. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
     redefinition of unused 'DiffSet' from line 49
    
  8. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
     redefinition of unused 'get_patched_file' from line 43
    
  9. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
     redefinition of unused 'get_original_file' from line 43
    
  10. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (96 > 79 characters)
    
  11. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (100 > 79 characters)
    
  12. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  13. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  14. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 22
     E231 missing whitespace after ':'
    
  15. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 43
     E231 missing whitespace after ':'
    
  16. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  17. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 35
     E128 continuation line under-indented for visual indent
    
  18. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (94 > 79 characters)
    
  19. 
      
TI
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
  2. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/diffviewer/errors.py (Diff revision 4)
     
     
    Show all issues
    Col: 44
     W292 no newline at end of file
    
  4. reviewboard/reviews/urls.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. reviewboard/reviews/urls.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
     redefinition of unused 'get_object_or_404' from line 18
    
  7. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
     redefinition of unused 'DiffSet' from line 49
    
  8. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
     redefinition of unused 'get_original_file' from line 43
    
  9. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
     redefinition of unused 'get_patched_file' from line 43
    
  10. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (96 > 79 characters)
    
  11. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  12. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (100 > 79 characters)
    
  13. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  14. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  15. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (90 > 79 characters)
    
  16. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  17. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  18. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  19. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
     local variable 'new_file' is assigned to but never used
    
  20. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (91 > 79 characters)
    
  21. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (93 > 79 characters)
    
  22. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  23. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 17
     E225 missing whitespace around operator
    
  24. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 38
     E231 missing whitespace after ':'
    
  25. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 29
     E231 missing whitespace after ':'
    
  26. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 32
     E231 missing whitespace after ':'
    
  27. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  28. 
      
brennie
  1. Hi Tien. This is just a reminder that when you upload a new diff revision, you should mark old issues from Review Bot as fixed (if you fixed them) or dropped. Otherwise its hard to keep track of how many issues are with the current diff revisision. Please also fix the errors that Review Bot complains about.

  2. 
      
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
  2. reviewboard/diffviewer/diffutils.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
     redefinition of unused 'DiffSet' from line 49
    
  4. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
     redefinition of unused 'get_patched_file' from line 43
    
  5. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
     redefinition of unused 'get_original_file' from line 43
    
  6. 
      
TI
TI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
  2. reviewboard/diffviewer/diffutils.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
     redefinition of unused 'DiffSet' from line 49
    
  3. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
     redefinition of unused 'get_original_file' from line 43
    
  4. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
     redefinition of unused 'get_patched_file' from line 43
    
  5. 
      
TI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
  2. reviewboard/diffviewer/diffutils.py (Diff revision 7)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
     redefinition of unused 'DiffSet' from line 49
    
  4. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
     redefinition of unused 'get_original_file' from line 43
    
  5. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
     redefinition of unused 'get_patched_file' from line 43
    
  6. 
      
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 8)
     
     
    Show all issues

    Undo this change.

  3. reviewboard/diffviewer/diffutils.py (Diff revision 8)
     
     
     
    Show all issues

    Use all available line width (79 characters).

    1. the whole thing is 80 characters. is it recommended to "balance out" the lines, or maximize use of any one line?

  4. reviewboard/diffviewer/errors.py (Diff revision 8)
     
     
     
     
     
    Show all issues

    We use single quotes. Also, if you wrap the string in parenthesis you can leave out the backslashes (which are ugly and can cause problems if there is trailing whitespace), e.g.,

    error_msg = ("line 1"
                 "line 2"
                 "..."
                 "line n")
    
  5. reviewboard/diffviewer/errors.py (Diff revision 8)
     
     
     
     
     
     
    Show all issues

    This should all be lined up with the opening quote on the previous lines.

  6. reviewboard/diffviewer/errors.py (Diff revision 8)
     
     
    Show all issues

    You want to do:

    super(PatchError, self).__init__(error_msg)
    

    and it is usually done at the start of the overriden __init__.

  7. reviewboard/diffviewer/views.py (Diff revision 8)
     
     
    Show all issues

    Who did what can be shown with git blame so this isn't really needed.

  8. reviewboard/diffviewer/views.py (Diff revision 8)
     
     
    Show all issues

    This method needs a docstring.

  9. reviewboard/diffviewer/views.py (Diff revision 8)
     
     
     
    Show all issues

    This seems like good information for the docstring.

  10. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues

    You should use from django.utils.six import StringIO. It would probably be better to use cStringIO Ă  la from django.utils.six.moves import cStringIO

    1. turned out django's six is the exact same as the pip install six module. cool

  11. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues

    Order these alphabetically.

  12. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues

    Order these alphabetically.

  13. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues

    This class needs a docstring.

  14. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues

    This method needs a docstring.

  15. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
    Show all issues

    You can format this as:

            self.review_request, response = _find_review_request(
                request, review_request_id, None)
    
  16. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues

    Why does this assign to self if there are no other methods in that will use it?

    1. good point. I previously had a different get_context_data and render_to_response methods, which needed that, so I dumped review_request into self. It's not needed anymore

  17. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
    Show all issues

    Does this take into account local sites?

    1. I'm not 100% sure, but it seems to be the pattern every other View in this class is following:

          self.review_request, response = \
              _find_review_request(request, review_request_id, None)
      
          if not self.review_request:
              return response
      
          draft = self.review_request.get_draft(request.user)
      

      (then grab diffsets and whatnot...)

      What is taking local sites into account?

  18. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues

    Comments generally are written in second person (We...) or like follows: There shouldn't be more than one diff returned.

    Also, since we are assuming this, it is probably good to assert our assumption.

    diff_files = get_diff_files(diffset, filediff, None, request)
    assert len(diff_files) == 1
    diff_file = diff_files[0]
    
  19. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
    Show all issues

    Since the error handling is the same you can do

    except (FileNotFoundError, SCMError) as e:
       # common error handling
    
    1. FileNotFoundError is actually a subclass of SCMError. I intend, for later versions, to do something different with the FileNotFoundError instead.

  20. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
    Show all issues

    You don't implement render_to_response so why are you explicitly invoking the super method?

    Also, this formatting is wonky. How about:
    return super(PatchErrorDownloadView, self).render_to_response(
    {
    'error': e
    })

    1. I did this because this "view" doesn't actually return a view except in this case anyway. I wanted to just use TemplateView's rendering function

  21. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
    Show all issues

    Same as previous comment.

  22. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues

    We use single quotes for strings. Also this will have to be localized.

  23. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    It would probably be better to use a StreamingHttpResponse here so we don't block while the zip is built.

  24. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
     
     
     
     
    Show all issues

    We use single quotes for strings.

  25. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues

    This method needs a docstring.

  26. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
     
    Show all issues

    This information seems like it would be good for the docstring instead of as just a comment.

  27. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues

    No blank line here.

  28. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
    Show all issues

    The braces should be on their own lines. Each key-value pair should be its own line. The last key-value pair should have a trailing comma.

    We use single quotes for strings.

  29. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
     
    Show all issues

    We use single quotes for strings.

  30. Show all issues

    Strings need to be translated. Here and elsewhere. I also don't think this is a good title -- perhaps just Diff Doctor?

  31. Show all issues

    This doesn't seem like a good string to put here. Also, make sure all strings are localized.

  32. 
      
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 9)
     
     
    Show all issues
    Col: 13
     E123 closing bracket does not match indentation of opening bracket's line
    
  3. 
      
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues
    Col: 13
     E123 closing bracket does not match indentation of opening bracket's line
    
  3. 
      
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/reviews/views.py (Diff revisions 8 - 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    I mentioned in my last review that since you are not using overriding render_to_response, you should just use self.render_to_response. Since you are subclassing from the TemplateView, self.render_to_response will have the exact same definition as TemplateView.render_to_response and will make for less line noise.

    Also, you can put the { on the previous line, like so:

                return self.render_to_response({
                    'foo': 'bar',
                })
    
    1. ah, yes. how could I have totally forgotten that I can just do self.render_to_response?

  3. reviewboard/reviews/views.py (Diff revisions 8 - 11)
     
     
    Show all issues

    Single quotes for strings. Should have a trailing comma.

  4. reviewboard/reviews/views.py (Diff revisions 8 - 11)
     
     
    Show all issues

    Single quotes for strings. Should have a trailing comma.

  5. reviewboard/reviews/views.py (Diff revisions 8 - 11)
     
     
    Show all issues

    Single quotes for strings. Should have a trailing comma.

    The %-substitution should happen outside the call to _. Otherwise, multiple strings have to be translted instead of one containing a %s.

    You are also missing a trailing period in the string.

    1. isn't the _(string) call only called once?

    2. Yes. However, if your string is _('foo %d' % bar), it will fail to look up a translation unless you have one for every value of bar. That is why you should do _('foo %d') % bar.

      For example, if bar can be '1' or '2', then you need two seperate translations, one for 'foo 1' and one for 'foo 2'. If you do the %-substitution after you do the translation, you only need a single translation for 'foo %d'.

      Also, since filediff_id is an integer, you should use %d and not %s.

    3. ah, ok, that makes sense now

  6. reviewboard/templates/diffviewer/diff_fragment_error.html (Diff revisions 8 - 11)
     
     
     
     
    Show all issues

    Why not just {% trans "There was a problem with your patch." %}. Also, missing a period.

  7. reviewboard/templates/diffviewer/diff_fragment_error.html (Diff revisions 8 - 11)
     
     
     
     
    Show all issues

    You can just use {% trans error.msg %}

  8. reviewboard/templates/diffviewer/patch_error.html (Diff revisions 8 - 11)
     
     
     
     
    Show all issues

    is {{error}} expected to be HTML? If not, just use {% trans error %}.

  9. 
      
TI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
  2. 
      
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/renderers.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/renderers.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/templates/diffviewer/patch_error.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Show all issues
     'HttpResponseServerError' imported but unused
    
  3. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Show all issues
     'render' imported but unused
    
  4. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Show all issues
     'FileNotFoundError' imported but unused
    
  5. 
      
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/renderers.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/TEST
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/renderers.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
        reviewboard/TEST
    
    
  2. reviewboard/reviews/views.py (Diff revision 14)
     
     
    Show all issues
     'SCMError' imported but unused
    
  3. 
      
TI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/renderers.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/renderers.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
  2. 
      
TI
chipx86
  1. Looking good, overall!

    The majority of the issues here are more about following existing style conventions, and should be easy to clean up quickly.

    I'd love to see some screenshots on the review request as well.

  2. reviewboard/diffviewer/diffutils.py (Diff revision 15)
     
     
     
    Show all issues

    Imports should be in alphabetical order.

  3. reviewboard/diffviewer/errors.py (Diff revision 15)
     
     
    Show all issues

    Can you add a docstring for this?

  4. reviewboard/diffviewer/errors.py (Diff revision 15)
     
     
     
     
     
    Show all issues

    Considering we'll only be accessing this data if we're downloading it, it's a bit expensive to do every time we're throwing an error. Instead, the exception should be providing all the information needed for the download view to read and return the file contents.

  5. reviewboard/diffviewer/errors.py (Diff revision 15)
     
     
    Show all issues

    Comments should be in sentence casing and end with a period.

  6. reviewboard/diffviewer/errors.py (Diff revision 15)
     
     
    Show all issues

    No need for mode=. You can just pass 'r'.

  7. reviewboard/diffviewer/errors.py (Diff revision 15)
     
     
    Show all issues

    We should localize this.

  8. reviewboard/diffviewer/errors.py (Diff revision 15)
     
     
    Show all issues

    Blank line before this.

  9. reviewboard/diffviewer/renderers.py (Diff revision 15)
     
     
     
     
    Show all issues

    This change isn't relevant, and should be undone.

  10. reviewboard/diffviewer/views.py (Diff revision 15)
     
     
    Show all issues

    Missing a trailing period.

  11. reviewboard/diffviewer/views.py (Diff revision 15)
     
     
     
     
     
     
    Show all issues

    This needs to follow the guidelines for docstrings we have elsewhere. No :param: or :return:, either.

  12. reviewboard/reviews/urls.py (Diff revision 15)
     
     
     
     
    Show all issues

    Must be in alphabetical order.

  13. reviewboard/reviews/urls.py (Diff revision 15)
     
     
    Show all issues

    Should use dashes instead of underscores in the URL.

    This is also missing a $ at the end of the pattern.

  14. reviewboard/reviews/urls.py (Diff revision 15)
     
     
    Show all issues

    We use dashes instead of underscores for URL names.

  15. reviewboard/reviews/views.py (Diff revision 15)
     
     
     
    Show all issues

    The text must be on the first line.

    Instead of "This class ...", say something like "Downloads debug information for a failed patch."

  16. reviewboard/reviews/views.py (Diff revision 15)
     
     
     
    Show all issues

    This information is already in the function, and is more of an implementation detail for the URL structure. I would focus the docstring on the sort of information provided by the view, and not the parameters to the URL.

  17. reviewboard/reviews/views.py (Diff revision 15)
     
     
     
     
    Show all issues

    Make sure to follow the proper docstring format:

    """One-line summary
    
    Multi-line description.
    """
    
  18. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Show all issues

    Your code isn't actually streaming the content, so there's no reason to use a StreamingHttpResponse.

    A StreamingHttpResponse would be useful if you were to pass an instance of some sort of class that was fetching the contents of files on-demand. It could be written this way, but we've spent some time on this for another project, and it's just not easy for zip files.

    So, in this case, I'd use a HttpResponse.

  19. reviewboard/reviews/views.py (Diff revision 15)
     
     
     
    Show all issues

    There's not much benefit to using a format string here, if we're hard-coding the string to put in there.

  20. reviewboard/reviews/views.py (Diff revision 15)
     
     
     
     
     
    Show all issues

    If there's a different sort of exception, we need to know about it. This should definitely remain a 500.

    It's fine to just log the exception information here and re-raise. That way, the caller will see a 500 when something unexpected goes wrong, and an administrator can look in the log file for the details.

  21. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Show all issues

    This needs a docstring.

  22. reviewboard/reviews/views.py (Diff revision 15)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    You can write this as one statement:

    return local_site_reverse(
        'patch-download',
        request=self.request,
        kwargs={
            ...
        })
    
  23. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Show all issues

    That's not adding anything beyond the name of the function. The docstring should explain what information is being added.

  24. reviewboard/reviews/views.py (Diff revision 15)
     
     
     
    Show all issues

    Needs to be proper sentences.

  25. reviewboard/reviews/views.py (Diff revision 15)
     
     
     
     
     
     
    Show all issues

    This would be better as:

    error_data = (
        (e.file, 'old_file'),
        ...
    )
    
    for code, context_attr in error_data:
        ...
    
  26. Show all issues

    Make sure the indentation of all these template tags are correct, with respect to any parent template tags.

  27. Show all issues

    #oldfile is going to conflict with other errors on the page. Each ID needs to be unique. Note how we did it before with the details link.

    Also, the text needs to be localized.

    Same below.

  28. Show all issues

    It's best to call mark_safe() around the data being fed into the template, when we know it's safe.

  29. Show all issues

    These lines are way too short.

    Also, no spaces inside the {{...}}.

  30. 
      
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 16)
     
     
    Show all issues
     'HttpResponseServerError' imported but unused
    
  3. 
      
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
  2. reviewboard/diffviewer/views.py (Diff revision 17)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. reviewboard/diffviewer/views.py (Diff revision 17)
     
     
    Show all issues
    Col: 43
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/diffviewer/views.py (Diff revision 17)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. reviewboard/reviews/views.py (Diff revision 17)
     
     
    Show all issues
     'HttpResponseServerError' imported but unused
    
  6. reviewboard/reviews/views.py (Diff revision 17)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  7. 
      
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
        reviewboard/diffviewer/errors.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/diff_fragment_error.html
    
    
  2. 
      
david
Review request changed
Status:
Discarded
Change Summary:

New implementation at /r/8563/