-
-
-
-
reviewboard/diffviewer/views.py (Diff revision 1) local variable 'response' is assigned to but never used
-
reviewboard/diffviewer/views.py (Diff revision 1) Col: 17 E127 continuation line over-indented for visual indent
Add a better display for patch errors.
Review Request #8563 — Created Dec. 6, 2016 and submitted
When a file fails to patch, we would show a pretty terrible error message. It
would give the location of the temp directory with the original and rejects
files, and then theoretically show the error output from thepatch
command
(although in the case where hunks couldn't be applied, that error output
wouldn't be shown because it's displayed to stdout instead of stderr).This replaces that with a new display that actually shows the patch output
(with temporary directory names filtered out), and instead of forcing the
administrator to dig through that directory, allows the user to either show the
rejects inline on the page or download a .zip bundle containing the original
file, the new file, the diff, and the rejects.Based on work by Tien Vu.
- Uploaded a hand-modified patch that would fail to apply cleanly. Saw the new
output. Viewed rejects inline and checked the contents of the downloaded
error bundle. - Ran unit tests.
Description | From | Last Updated |
---|---|---|
'format_html' imported but unused |
![]() |
|
'local_site_reverse' imported but unused |
![]() |
|
local variable 'response' is assigned to but never used |
![]() |
|
Col: 17 E127 continuation line over-indented for visual indent |
![]() |
|
This will also catch, for example, KeyboardInterrupt, RumtimeError and other non-Exceptions that are meant to terminate the program. We should … |
|
|
Put the ) on the following line for symmetry? |
|
|
Can you put args starting on the next line indented by 4? |
|
|
Same here. |
|
|
And here. |
|
|
Can we add Args, Returns, and Raises while here? |
|
|
I missed this one. except Exception here too. |
|
|
In the try/except above, we set new_file = None in the except. For rejects, we're setting it before-hand. Let's move … |
|
|
Can we move this into the __init__ call? |
|
|
Swap these. |
|
|
Swap these. |
|
|
Is this change related to your work? Raising Http404 actually presents a 404 page, whereas returning HttpResponseNotFound returns an empty … |
|
|
Can we use local_site_reverse here? |
|
|
Let's give these names. |
|
|
Alphabetical. |
|
|
This can be removed. |
|

Change Summary:
Credit where credit is due.
Description: |
|
---|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+457 -152) |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/webapi/resources/base_patched_file.py reviewboard/reviews/urls.py reviewboard/diffviewer/errors.py reviewboard/diffviewer/views.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/diffviewer/diff_fragment_patch_error.html reviewboard/templates/diffviewer/diff_fragment_error.html reviewboard/templates/diffviewer/diff_fragment_error_base.html reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/webapi/resources/base_patched_file.py reviewboard/reviews/urls.py reviewboard/diffviewer/errors.py reviewboard/diffviewer/views.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/diffviewer/diff_fragment_patch_error.html reviewboard/templates/diffviewer/diff_fragment_error.html reviewboard/templates/diffviewer/diff_fragment_error_base.html reviewboard/static/rb/css/pages/diffviewer.less
-
-
reviewboard/diffviewer/diffutils.py (Diff revision 2) This will also catch, for example,
KeyboardInterrupt
,RumtimeError
and other non-Exception
s that are meant to terminate the program. We should be more specific here or just catchException
. -
-
reviewboard/diffviewer/views.py (Diff revision 2) Can you put args starting on the next line indented by 4?
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+463 -157) |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/webapi/resources/base_patched_file.py reviewboard/reviews/urls.py reviewboard/diffviewer/errors.py reviewboard/diffviewer/views.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/diffviewer/diff_fragment_patch_error.html reviewboard/templates/diffviewer/diff_fragment_error.html reviewboard/templates/diffviewer/diff_fragment_error_base.html reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/webapi/resources/base_patched_file.py reviewboard/reviews/urls.py reviewboard/diffviewer/errors.py reviewboard/diffviewer/views.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/diffviewer/diff_fragment_patch_error.html reviewboard/templates/diffviewer/diff_fragment_error.html reviewboard/templates/diffviewer/diff_fragment_error_base.html reviewboard/static/rb/css/pages/diffviewer.less
-
-
reviewboard/diffviewer/diffutils.py (Diff revision 3) Can we add
Args
,Returns
, andRaises
while here? -
reviewboard/diffviewer/diffutils.py (Diff revision 3) In the
try/except
above, we setnew_file = None
in theexcept
. Forrejects
, we're setting it before-hand. Let's move it into theexcept
here. -
-
-
-
reviewboard/diffviewer/views.py (Diff revision 3) Is this change related to your work?
Raising
Http404
actually presents a 404 page, whereas returningHttpResponseNotFound
returns an empty 404. -
-
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+491 -158) |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/webapi/resources/base_patched_file.py reviewboard/reviews/urls.py reviewboard/diffviewer/errors.py reviewboard/diffviewer/views.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/diffviewer/diff_fragment_patch_error.html reviewboard/templates/diffviewer/diff_fragment_error.html reviewboard/templates/diffviewer/diff_fragment_error_base.html reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/webapi/resources/base_patched_file.py reviewboard/reviews/urls.py reviewboard/diffviewer/errors.py reviewboard/diffviewer/views.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/diffviewer/diff_fragment_patch_error.html reviewboard/templates/diffviewer/diff_fragment_error.html reviewboard/templates/diffviewer/diff_fragment_error_base.html reviewboard/static/rb/css/pages/diffviewer.less