Summary

Diff Doctor

Review Request #6993 — Created Feb. 27, 2015 and discarded Dec. 6, 2016, 4:05 p.m.

Information

Owner

tienv

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

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

Issues

Description	From	Last Updated
redefinition of unused 'get_object_or_404' from line 18	reviewbot	March 7, 2015, 6:23 p.m.
redefinition of unused 'DiffSet' from line 49	reviewbot	March 7, 2015, 6:23 p.m.
Col: 80 E501 line too long (96 > 79 characters)	reviewbot	March 7, 2015, 6:23 p.m.
redefinition of unused 'get_patched_file' from line 43	reviewbot	March 7, 2015, 6:23 p.m.
redefinition of unused 'get_original_file' from line 43	reviewbot	March 7, 2015, 6:27 p.m.
Col: 80 E501 line too long (100 > 79 characters)	reviewbot	March 7, 2015, 6:23 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	March 7, 2015, 6:23 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 7, 2015, 6:23 p.m.
Col: 43 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:23 p.m.
Col: 22 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:23 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	March 7, 2015, 6:23 p.m.
Col: 35 E128 continuation line under-indented for visual indent	reviewbot	March 7, 2015, 6:23 p.m.
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	March 7, 2015, 6:23 p.m.
Col: 34 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:23 p.m.
Col: 25 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:23 p.m.
Col: 28 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:23 p.m.
Col: 80 E501 line too long (88 > 79 characters)	reviewbot	March 7, 2015, 6:23 p.m.
Col: 34 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:23 p.m.
Col: 25 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:23 p.m.
Col: 28 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:23 p.m.
Col: 80 E501 line too long (88 > 79 characters)	reviewbot	March 7, 2015, 6:23 p.m.
redefinition of unused 'get_object_or_404' from line 18	reviewbot	March 7, 2015, 6:23 p.m.
redefinition of unused 'DiffSet' from line 49	reviewbot	March 7, 2015, 6:24 p.m.
redefinition of unused 'get_patched_file' from line 43	reviewbot	March 7, 2015, 6:23 p.m.
redefinition of unused 'get_original_file' from line 43	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (96 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (100 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 22 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:24 p.m.
Col: 43 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:27 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 35 E128 continuation line under-indented for visual indent	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 44 W292 no newline at end of file	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
redefinition of unused 'get_object_or_404' from line 18	reviewbot	March 7, 2015, 6:24 p.m.
redefinition of unused 'DiffSet' from line 49	reviewbot	March 7, 2015, 6:24 p.m.
redefinition of unused 'get_original_file' from line 43	reviewbot	March 7, 2015, 6:24 p.m.
redefinition of unused 'get_patched_file' from line 43	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (96 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	March 7, 2015, 6:27 p.m.
Col: 80 E501 line too long (100 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (90 > 79 characters)	reviewbot	March 7, 2015, 6:27 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
local variable 'new_file' is assigned to but never used	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (91 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (93 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (88 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 17 E225 missing whitespace around operator	reviewbot	March 7, 2015, 6:24 p.m.
Col: 38 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:24 p.m.
Col: 29 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:24 p.m.
Col: 32 E231 missing whitespace after ':'	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	March 7, 2015, 6:24 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 7, 2015, 6:32 p.m.
redefinition of unused 'DiffSet' from line 49	reviewbot	March 7, 2015, 6:32 p.m.
redefinition of unused 'get_patched_file' from line 43	reviewbot	March 7, 2015, 6:32 p.m.
redefinition of unused 'get_original_file' from line 43	reviewbot	March 7, 2015, 6:32 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 11, 2015, 8:26 p.m.
redefinition of unused 'DiffSet' from line 49	reviewbot	March 11, 2015, 8:22 p.m.
redefinition of unused 'get_original_file' from line 43	reviewbot	March 11, 2015, 8:22 p.m.
redefinition of unused 'get_patched_file' from line 43	reviewbot	March 11, 2015, 8:22 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 11, 2015, 8:23 p.m.
redefinition of unused 'DiffSet' from line 49	reviewbot	March 11, 2015, 8:23 p.m.
redefinition of unused 'get_original_file' from line 43	reviewbot	March 11, 2015, 8:23 p.m.
redefinition of unused 'get_patched_file' from line 43	reviewbot	March 11, 2015, 8:23 p.m.
Undo this change.	brennie	March 13, 2015, 8:17 p.m.
Use all available line width (79 characters).	brennie	March 16, 2015, 7:34 p.m.
We use single quotes. Also, if you wrap the string in parenthesis you can leave out the backslashes (which are …	brennie	March 13, 2015, 8:24 p.m.
This should all be lined up with the opening quote on the previous lines.	brennie	March 13, 2015, 8:24 p.m.
You want to do: super(PatchError, self).__init__(error_msg) and it is usually done at the start of the overriden __init__.	brennie	March 13, 2015, 8:25 p.m.
Who did what can be shown with git blame so this isn't really needed.	brennie	March 13, 2015, 8:26 p.m.
This method needs a docstring.	brennie	March 13, 2015, 8:27 p.m.
This seems like good information for the docstring.	brennie	March 13, 2015, 8:30 p.m.
You should use from django.utils.six import StringIO. It would probably be better to use cStringIO à la from django.utils.six.moves import …	brennie	March 13, 2015, 8:42 p.m.
Order these alphabetically.	brennie	March 13, 2015, 8:44 p.m.
Order these alphabetically.	brennie	March 13, 2015, 8:44 p.m.
This class needs a docstring.	brennie	March 13, 2015, 8:52 p.m.
This method needs a docstring.	brennie	March 13, 2015, 8:52 p.m.
You can format this as: self.review_request, response = _find_review_request( request, review_request_id, None)	brennie	March 13, 2015, 8:57 p.m.
Why does this assign to self if there are no other methods in that will use it?	brennie	March 13, 2015, 8:58 p.m.
Does this take into account local sites?	brennie	March 16, 2015, 7:34 p.m.
Comments generally are written in second person (We...) or like follows: There shouldn't be more than one diff returned. Also, …	brennie	March 13, 2015, 8:58 p.m.
Since the error handling is the same you can do except (FileNotFoundError, SCMError) as e: # common error handling	brennie	March 16, 2015, 7:34 p.m.
You don't implement render_to_response so why are you explicitly invoking the super method? Also, this formatting is wonky. How about: …	brennie	March 16, 2015, 7:34 p.m.
Same as previous comment.	brennie	March 13, 2015, 9:01 p.m.
We use single quotes for strings. Also this will have to be localized.	brennie	March 13, 2015, 9:03 p.m.
It would probably be better to use a StreamingHttpResponse here so we don't block while the zip is built.	brennie	March 13, 2015, 9:26 p.m.
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 …	brennie	March 20, 2015, 10:25 p.m.
Single quotes for strings. Should have a trailing comma.	brennie	March 20, 2015, 10:29 p.m.
Single quotes for strings. Should have a trailing comma.	brennie	March 20, 2015, 10:29 p.m.
Single quotes for strings. Should have a trailing comma. The %-substitution should happen outside the call to _. Otherwise, multiple …	brennie	March 20, 2015, 11:11 p.m.
We use single quotes for strings.	brennie	March 13, 2015, 9:27 p.m.
This method needs a docstring.	brennie	March 13, 2015, 9:29 p.m.
This information seems like it would be good for the docstring instead of as just a comment.	brennie	March 13, 2015, 9:29 p.m.
No blank line here.	brennie	March 13, 2015, 9:29 p.m.
The braces should be on their own lines. Each key-value pair should be its own line. The last key-value pair …	brennie	March 13, 2015, 9:30 p.m.
We use single quotes for strings.	brennie	March 13, 2015, 9:30 p.m.
Strings need to be translated. Here and elsewhere. I also don't think this is a good title -- perhaps just …	brennie	March 13, 2015, 9:40 p.m.
Why not just {% trans "There was a problem with your patch." %}. Also, missing a period.	brennie	March 20, 2015, 10:34 p.m.
You can just use {% trans error.msg %}	brennie	March 20, 2015, 10:34 p.m.
This doesn't seem like a good string to put here. Also, make sure all strings are localized.	brennie	March 13, 2015, 9:41 p.m.
is {{error}} expected to be HTML? If not, just use {% trans error %}.	brennie	March 20, 2015, 10:32 p.m.
Col: 13 E123 closing bracket does not match indentation of opening bracket's line	reviewbot	March 16, 2015, 7:34 p.m.
Col: 13 E123 closing bracket does not match indentation of opening bracket's line	reviewbot	March 16, 2015, 7:34 p.m.
'HttpResponseServerError' imported but unused	reviewbot	March 20, 2015, 11:12 p.m.
'render' imported but unused	reviewbot	March 20, 2015, 11:12 p.m.
'FileNotFoundError' imported but unused	reviewbot	March 20, 2015, 11:12 p.m.
'SCMError' imported but unused	reviewbot	April 1, 2015, 6:20 p.m.
Imports should be in alphabetical order.	chipx86	April 2, 2015, 4:34 p.m.
Can you add a docstring for this?	chipx86	April 2, 2015, 4:36 p.m.
Considering we'll only be accessing this data if we're downloading it, it's a bit expensive to do every time we're …	chipx86	April 2, 2015, 4:41 p.m.
Comments should be in sentence casing and end with a period.	chipx86	April 2, 2015, 4:42 p.m.
No need for mode=. You can just pass 'r'.	chipx86	April 2, 2015, 4:42 p.m.
We should localize this.	chipx86	April 2, 2015, 4:44 p.m.
Blank line before this.	chipx86	April 2, 2015, 4:44 p.m.
This change isn't relevant, and should be undone.	chipx86	April 2, 2015, 4:45 p.m.
Missing a trailing period.	chipx86	April 2, 2015, 4:45 p.m.
This needs to follow the guidelines for docstrings we have elsewhere. No :param: or :return:, either.	chipx86	April 2, 2015, 4:45 p.m.
Must be in alphabetical order.	chipx86	April 2, 2015, 4:46 p.m.
Should use dashes instead of underscores in the URL. This is also missing a $ at the end of the …	chipx86	April 2, 2015, 4:48 p.m.
We use dashes instead of underscores for URL names.	chipx86	April 2, 2015, 4:48 p.m.
The text must be on the first line. Instead of "This class ...", say something like "Downloads debug information for …	chipx86	April 2, 2015, 4:49 p.m.
This information is already in the function, and is more of an implementation detail for the URL structure. I would …	chipx86	April 2, 2015, 4:49 p.m.
Make sure to follow the proper docstring format: """One-line summary Multi-line description. """	chipx86	April 2, 2015, 4:50 p.m.
Your code isn't actually streaming the content, so there's no reason to use a StreamingHttpResponse. A StreamingHttpResponse would be useful …	chipx86	April 2, 2015, 4:51 p.m.
There's not much benefit to using a format string here, if we're hard-coding the string to put in there.	chipx86	April 2, 2015, 4:52 p.m.
If there's a different sort of exception, we need to know about it. This should definitely remain a 500. It's …	chipx86	April 2, 2015, 4:55 p.m.
This needs a docstring.	chipx86	April 2, 2015, 4:56 p.m.
You can write this as one statement: return local_site_reverse( 'patch-download', request=self.request, kwargs={ ... })	chipx86	April 2, 2015, 4:59 p.m.
That's not adding anything beyond the name of the function. The docstring should explain what information is being added.	chipx86	April 2, 2015, 5 p.m.
Needs to be proper sentences.	chipx86	April 2, 2015, 5 p.m.
This would be better as: error_data = ( (e.file, 'old_file'), ... ) for code, context_attr in error_data: ...	chipx86	April 2, 2015, 5:02 p.m.
Make sure the indentation of all these template tags are correct, with respect to any parent template tags.	chipx86	April 2, 2015, 6:42 p.m.
#oldfile is going to conflict with other errors on the page. Each ID needs to be unique. Note how we …	chipx86	April 2, 2015, 6:35 p.m.
It's best to call mark_safe() around the data being fed into the template, when we know it's safe.	chipx86	April 2, 2015, 6:41 p.m.
These lines are way too short. Also, no spaces inside the {{...}}.	chipx86	April 2, 2015, 6:42 p.m.
'HttpResponseServerError' imported but unused	reviewbot	April 2, 2015, 8:01 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Sept. 12, 2015, 5:44 p.m.
Col: 43 E128 continuation line under-indented for visual indent	reviewbot	Sept. 12, 2015, 5:44 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Sept. 12, 2015, 5:44 p.m.
'HttpResponseServerError' imported but unused	reviewbot	Sept. 12, 2015, 5:44 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Sept. 12, 2015, 5:44 p.m.

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

reviewboard/reviews/views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 redefinition of unused 'get_object_or_404' from line 18
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 redefinition of unused 'DiffSet' from line 49
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (96 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 redefinition of unused 'get_patched_file' from line 43
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 redefinition of unused 'get_original_file' from line 43
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (100 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 43
 E231 missing whitespace after ':'
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 22
 E231 missing whitespace after ':'
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 35
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (94 > 79 characters)
```

Commit:

cfa84200e5e5d219a195a0e961019199668e06be

3a0c39547594dc8e5e8120cb6ea52a4d57619125

Diff:

Revision 2 (+24 -1)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/templates/diffviewer/diff_fragment_error.html

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

reviewboard/diffviewer/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 34
 E231 missing whitespace after ':'
```
reviewboard/diffviewer/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 25
 E231 missing whitespace after ':'
```
reviewboard/diffviewer/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 28
 E231 missing whitespace after ':'
```
reviewboard/diffviewer/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (88 > 79 characters)
```

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:

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

A URL in reviews for this. I suggest putting this in the dowload_diff_urls section as '^patch-results/$'.
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.
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.

Diff:

Revision 3 (+70 -1)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/templates/diffviewer/diff_fragment_error.html

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

reviewboard/diffviewer/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 34
 E231 missing whitespace after ':'
```
reviewboard/diffviewer/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 25
 E231 missing whitespace after ':'
```
reviewboard/diffviewer/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 28
 E231 missing whitespace after ':'
```
reviewboard/diffviewer/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (88 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
 redefinition of unused 'get_object_or_404' from line 18
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
 redefinition of unused 'DiffSet' from line 49
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
 redefinition of unused 'get_patched_file' from line 43
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
 redefinition of unused 'get_original_file' from line 43
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (96 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (100 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 22
 E231 missing whitespace after ':'
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 43
 E231 missing whitespace after ':'
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 35
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (94 > 79 characters)
```

Summary:: Diff Doctor Draft
(WIP) Diff Doctor Draft

Commit:

3a0c39547594dc8e5e8120cb6ea52a4d57619125

b6bdc1f483c8b1d5fa89899e7587dbc9d1f60938

Diff:

Revision 4 (+185 -13)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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



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

reviewboard/diffviewer/diffutils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/errors.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 44
 W292 no newline at end of file
```
reviewboard/reviews/urls.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/urls.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 redefinition of unused 'get_object_or_404' from line 18
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 redefinition of unused 'DiffSet' from line 49
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 redefinition of unused 'get_original_file' from line 43
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 redefinition of unused 'get_patched_file' from line 43
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (96 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (100 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (90 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 local variable 'new_file' is assigned to but never used
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (91 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (93 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (88 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 17
 E225 missing whitespace around operator
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 38
 E231 missing whitespace after ':'
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 29
 E231 missing whitespace after ':'
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 32
 E231 missing whitespace after ':'
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```

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.

Commit:

b6bdc1f483c8b1d5fa89899e7587dbc9d1f60938

81d9a173ca1e287a91790b5bc81307bc91395dda

Diff:

Revision 5 (+165 -17)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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



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

reviewboard/diffviewer/diffutils.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 redefinition of unused 'DiffSet' from line 49
```
reviewboard/reviews/views.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 redefinition of unused 'get_patched_file' from line 43
```
reviewboard/reviews/views.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 redefinition of unused 'get_original_file' from line 43
```

Change Summary:: Copy paste from commit message:

Move location of diff file rejection download Summary of changes: - Deleted PatchErrorView, instead rendering content in ReviewsDiffFragmentView - Simplified get() function in PatchErrorDownloadView - Add exception handling for SCMErrors (which are in turn thrown when HTTPErrors are thr - Added a wrapper in DiffFragmentView to let subclasses insert extra context variables w - Change template code in diff_fragment_error.html to display Diff Doctor information
Description:: ~
Seeing if I'm even on the right track
~
WIP

Diff:

Revision 6 (+165 -17)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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

reviewboard/diffviewer/diffutils.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

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

reviewboard/reviews/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
 redefinition of unused 'DiffSet' from line 49
```
reviewboard/reviews/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
 redefinition of unused 'get_original_file' from line 43
```
reviewboard/reviews/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
 redefinition of unused 'get_patched_file' from line 43
```

Change Summary:

blah

Diff:

Revision 7 (+165 -17)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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



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

reviewboard/diffviewer/diffutils.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been resolved. Show all issues
```
 redefinition of unused 'DiffSet' from line 49
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been resolved. Show all issues
```
 redefinition of unused 'get_original_file' from line 43
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been resolved. Show all issues
```
 redefinition of unused 'get_patched_file' from line 43
```

Summary:

(WIP) Diff Doctor Draft

Diff Doctor - Stage 1

Description:

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

Testing Done:

	+	Manual testing has been done, for:
	+	- Normal review requests (no diff error)
	+	- 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)
	+	- 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.

Commit:

81d9a173ca1e287a91790b5bc81307bc91395dda

84ce8178e429f05ed75001f0ed87c983882b9854

Diff:

Revision 8 (+161 -19)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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



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

reviewboard/diffviewer/diffutils.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Undo this change.
```

reviewboard/diffviewer/diffutils.py (Diff revision 8)

The issue has been resolved. Show all issues

Use all available line width (79 characters).

tienv March 13, 2015, 9:42 p.m.

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

reviewboard/diffviewer/errors.py (Diff revision 8)

The issue has been resolved. 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")

reviewboard/diffviewer/errors.py (Diff revision 8)

The issue has been resolved. Show all issues

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

reviewboard/diffviewer/errors.py (Diff revision 8)

The issue has been resolved. 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__.

reviewboard/diffviewer/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Who did what can be shown with git blame so this isn't really needed.
```
reviewboard/diffviewer/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
This method needs a docstring.
```
reviewboard/diffviewer/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
This seems like good information for the docstring.
```

reviewboard/reviews/views.py (Diff revision 8)

The issue has been resolved. 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

tienv March 13, 2015, 9:42 p.m.

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

reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Order these alphabetically.
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Order these alphabetically.
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
This class needs a docstring.
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
This method needs a docstring.
```

reviewboard/reviews/views.py (Diff revision 8)

The issue has been resolved. Show all issues

You can format this as:

        self.review_request, response = _find_review_request(
            request, review_request_id, None)

reviewboard/reviews/views.py (Diff revision 8)

The issue has been resolved. Show all issues

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

tienv March 13, 2015, 9:42 p.m.

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

reviewboard/reviews/views.py (Diff revision 8)

The issue has been dropped. Show all issues

Does this take into account local sites?

tienv March 13, 2015, 9:42 p.m.

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?

reviewboard/reviews/views.py (Diff revision 8)

The issue has been dropped. 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]

reviewboard/reviews/views.py (Diff revision 8)

The issue has been dropped. Show all issues

Since the error handling is the same you can do

except (FileNotFoundError, SCMError) as e:
   # common error handling

tienv March 13, 2015, 9:42 p.m.

FileNotFoundError is actually a subclass of SCMError. I intend, for later versions, to do something different with the FileNotFoundError instead.

reviewboard/reviews/views.py (Diff revision 8)

The issue has been resolved. 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

                    })

tienv March 13, 2015, 9:42 p.m.

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

reviewboard/reviews/views.py (Diff revision 8)
The issue has been dropped. Show all issues
```
Same as previous comment.
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
We use single quotes for strings. Also this will have to be localized.
```

reviewboard/reviews/views.py (Diff revision 8)

The issue has been resolved. Show all issues

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

reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
We use single quotes for strings.
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
This method needs a docstring.
```

reviewboard/reviews/views.py (Diff revision 8)

The issue has been resolved. Show all issues

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

reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No blank line here.
```

reviewboard/reviews/views.py (Diff revision 8)

The issue has been resolved. 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.

reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
We use single quotes for strings.
```

reviewboard/templates/diffviewer/diff_fragment_error.html (Diff revision 8)

The issue has been resolved. 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?

reviewboard/templates/diffviewer/patch_error.html (Diff revision 8)
The issue has been resolved. Show all issues
```
This doesn't seem like a good string to put here. Also, make sure all strings are localized.
```

Commit:

84ce8178e429f05ed75001f0ed87c983882b9854

bc5ed4d8e69eba54dbc60bae5068809747bd22d1

Diff:

Revision 9 (+219 -50)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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



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

reviewboard/reviews/views.py (Diff revision 9)

The issue has been resolved. Show all issues

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

Diff:

Revision 10 (+219 -50)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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



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

reviewboard/reviews/views.py (Diff revision 10)

The issue has been resolved. Show all issues

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

Commit:

bc5ed4d8e69eba54dbc60bae5068809747bd22d1

f732cd153ed94bbe590186d83e5dcc54dbb0ebfb

Diff:

Revision 11 (+219 -50)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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



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

reviewboard/reviews/views.py (Diff revisions 8 - 11)

The issue has been resolved. 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',
            })

tienv March 20, 2015, 10:34 p.m.

ah, yes. how could I have totally forgotten that I can just do self.render_to_response?

reviewboard/reviews/views.py (Diff revisions 8 - 11)
The issue has been resolved. Show all issues
```
Single quotes for strings. Should have a trailing comma.
```
reviewboard/reviews/views.py (Diff revisions 8 - 11)
The issue has been resolved. Show all issues
```
Single quotes for strings. Should have a trailing comma.
```

reviewboard/reviews/views.py (Diff revisions 8 - 11)

The issue has been resolved. 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.

tienv March 20, 2015, 10:34 p.m.

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

brennie March 20, 2015, 10:40 p.m.

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.

TI

tienv March 20, 2015, 11:11 p.m.
```
ah, ok, that makes sense now
```

reviewboard/templates/diffviewer/diff_fragment_error.html (Diff revisions 8 - 11)

The issue has been resolved. Show all issues

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

reviewboard/templates/diffviewer/diff_fragment_error.html (Diff revisions 8 - 11)

The issue has been resolved. Show all issues

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

reviewboard/templates/diffviewer/patch_error.html (Diff revisions 8 - 11)
The issue has been resolved. Show all issues
```
is {{error}} expected to be HTML? If not, just use {% trans error %}.
```

Commit:

f732cd153ed94bbe590186d83e5dcc54dbb0ebfb

0294ca350a7f71ae83fdabff00b35f55a439506a

Diff:

Revision 12 (+219 -50)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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



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

Commit:

0294ca350a7f71ae83fdabff00b35f55a439506a

e04e497607a5ed16d92490d6148c3ad9db01187e

Diff:

Revision 13 (+206 -26)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/templates/diffviewer/diff_fragment_error.html
	reviewboard/templates/diffviewer/patch_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
    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

reviewboard/reviews/views.py (Diff revision 13)
The issue has been resolved. Show all issues
```
 'HttpResponseServerError' imported but unused
```
reviewboard/reviews/views.py (Diff revision 13)
The issue has been resolved. Show all issues
```
 'render' imported but unused
```
reviewboard/reviews/views.py (Diff revision 13)
The issue has been resolved. Show all issues
```
 'FileNotFoundError' imported but unused
```

Commit:

e04e497607a5ed16d92490d6148c3ad9db01187e

755217e068c4a98bc57c7047b259a9d8b99757b6

Diff:

Revision 14 (+189 -27)

Show changes

	reviewboard/TEST
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	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
    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

reviewboard/reviews/views.py (Diff revision 14)
The issue has been resolved. Show all issues
```
 'SCMError' imported but unused
```

Commit:

755217e068c4a98bc57c7047b259a9d8b99757b6

26181f58eac1ebf34130c58b0548efddc16166f2

Diff:

Revision 15 (+193 -27)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/templates/diffviewer/diff_fragment_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



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

Summary:: Diff Doctor - Stage 1
Diff Doctor
Description:: 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 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.
Testing Done:: Manual testing has been done, for:
~ - Normal review requests (no diff error)
~ - 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)
~ - 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.
~ - 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

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.

reviewboard/diffviewer/diffutils.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Imports should be in alphabetical order.
```
reviewboard/diffviewer/errors.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Can you add a docstring for this?
```

reviewboard/diffviewer/errors.py (Diff revision 15)

The issue has been resolved. 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.

reviewboard/diffviewer/errors.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Comments should be in sentence casing and end with a period.
```
reviewboard/diffviewer/errors.py (Diff revision 15)
The issue has been resolved. Show all issues
```
No need for mode=. You can just pass 'r'.
```
reviewboard/diffviewer/errors.py (Diff revision 15)
The issue has been resolved. Show all issues
```
We should localize this.
```
reviewboard/diffviewer/errors.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Blank line before this.
```
reviewboard/diffviewer/renderers.py (Diff revision 15)
The issue has been resolved. Show all issues
```
This change isn't relevant, and should be undone.
```
reviewboard/diffviewer/views.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Missing a trailing period.
```

reviewboard/diffviewer/views.py (Diff revision 15)

The issue has been resolved. Show all issues

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

reviewboard/reviews/urls.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Must be in alphabetical order.
```

reviewboard/reviews/urls.py (Diff revision 15)

The issue has been resolved. Show all issues

Should use dashes instead of underscores in the URL.

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

reviewboard/reviews/urls.py (Diff revision 15)
The issue has been resolved. Show all issues
```
We use dashes instead of underscores for URL names.
```

reviewboard/reviews/views.py (Diff revision 15)

The issue has been resolved. 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."

reviewboard/reviews/views.py (Diff revision 15)

The issue has been resolved. 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.

reviewboard/reviews/views.py (Diff revision 15)

The issue has been resolved. Show all issues

Make sure to follow the proper docstring format:

"""One-line summary

Multi-line description.
"""

reviewboard/reviews/views.py (Diff revision 15)

The issue has been resolved. 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.

reviewboard/reviews/views.py (Diff revision 15)

The issue has been resolved. 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.

reviewboard/reviews/views.py (Diff revision 15)

The issue has been resolved. 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.

reviewboard/reviews/views.py (Diff revision 15)
The issue has been resolved. Show all issues
```
This needs a docstring.
```

reviewboard/reviews/views.py (Diff revision 15)

The issue has been resolved. Show all issues

You can write this as one statement:

return local_site_reverse(
    'patch-download',
    request=self.request,
    kwargs={
        ...
    })

reviewboard/reviews/views.py (Diff revision 15)

The issue has been resolved. Show all issues

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

reviewboard/reviews/views.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Needs to be proper sentences.
```

reviewboard/reviews/views.py (Diff revision 15)

The issue has been resolved. Show all issues

This would be better as:

error_data = (
    (e.file, 'old_file'),
    ...
)

for code, context_attr in error_data:
    ...

reviewboard/templates/diffviewer/diff_fragment_error.html (Diff revision 15)
The issue has been resolved. Show all issues
```
Make sure the indentation of all these template tags are correct, with respect to any parent template tags.
```

reviewboard/templates/diffviewer/diff_fragment_error.html (Diff revision 15)

The issue has been resolved. 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.

reviewboard/templates/diffviewer/diff_fragment_error.html (Diff revision 15)
The issue has been resolved. Show all issues
```
It's best to call mark_safe() around the data being fed into the template, when we know it's safe.
```
reviewboard/templates/diffviewer/diff_fragment_error.html (Diff revision 15)
The issue has been resolved. Show all issues
```
These lines are way too short.
Also, no spaces inside the {{...}}.
```

Commit:

26181f58eac1ebf34130c58b0548efddc16166f2

59c643070984a2eeb1bde84470abc2f43708792e

Diff:

Revision 16 (+192 -26)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/templates/diffviewer/diff_fragment_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



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/reviews/views.py (Diff revision 16)
The issue has been resolved. Show all issues
```
 'HttpResponseServerError' imported but unused
```

Diff:

Revision 17 (+192 -26)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/templates/diffviewer/diff_fragment_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



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/diffviewer/views.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/diffviewer/views.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Col: 43
 E128 continuation line under-indented for visual indent
```
reviewboard/diffviewer/views.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 17)
The issue has been resolved. Show all issues
```
 'HttpResponseServerError' imported but unused
```
reviewboard/reviews/views.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Diff:

Revision 18 (+193 -25)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/templates/diffviewer/diff_fragment_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



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

Status:: Discarded
Change Summary:: New implementation at /r/8563/