Fix blank diff fragments in interdiffs
Review Request #6838 — Created Jan. 29, 2015 and submitted
Previously, diff fragment comments from interdiffs were sometimes
resulting in a blank diff fragment. This was because of the diff
fragment viewer relying on the'total_line_count'value in the
filediff/interfilediff line counts. However, this value is not always
guaranteed to have a value. Now we specifically calculate the last
line in the filediff/interfilediff to avoid this issue.
Ran unit tests.
Verified that https://reviews.reviewboard.org/r/6815/#comment19874
does not appear as a blank diff with the patch applied. To do this I
saved and uploaded all the diffs in order and created an issue on the
same lines in the same interdiff.
| Description | From | Last Updated |
|---|---|---|
|
Looks like we're going from getting the count to the actual last line? Or is it the last line number? … |
 chipx86
|
-
Looks good in general. One question about naming.
More generally, can you structure the review request descriptions to be more high-level, less about the functions (some info there is fine for things like the docstrings). Basically, something along the lines of "Here's what wasn't working. Here's why. Here's how it's better." will help a lot when I go to draft release notes.
-
Looks like we're going from getting the count to the actual last line? Or is it the last line number? (If the latter, the function should probably be
get_last_line_number_in_diff).
- Change Summary:
-
Update à la Christian's issues.
- Description:
-
~ The file fetching portion of
diffutils.get_file_chunks_in_rangehas~ been refactored into the function diffutils.get_file_from_filediff.~ This function's docstring has also been updated for PEP8 compliance. ~ ~ Add the
diffutils.get_last_line_in_difffunction to determine the last~ (unified) line in a filediff and inter-filediff. This is used in the ~ Previously, diff fragment comments from interdiffs were sometimes
~ resulting in a blank diff fragment. This was because of the diff ~ fragment viewer relying on the 'total_line_count'value in the~ filediff/interfilediff line counts. However, this value is not always ~ guaranteed to have a value. Now we specifically calculate the last ~ line in the filediff/interfilediff to avoid this issue. - reviews.views.build_diff_comment_fragmentsfunction to calculate the- maximum line to expand to. Previously, the get_line_counts- method of the comment's FileDiffwas used, which did not always have a- value for 'total_line_count'. When this value wasNone, an HTTP 500- would occur and the diff fragment would appear empty.
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/diffviewer/diffutils.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/diffviewer/diffutils.py