Summary

Fix blank diff fragments in interdiffs

Review Request #6838 — Created Jan. 29, 2015 and submitted Feb. 1, 2015, 4:41 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

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.

Issues

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	Jan. 31, 2015, 10:16 a.m.

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

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.

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

The issue has been resolved. Show all issues

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_range` has
~		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_diff` function 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_fragments` function to calculate the
-		maximum line to expand to. Previously, the `get_line_counts`
-		method of the comment's `FileDiff` was used, which did not always have a
-		value for `'total_line_count'`. When this value was `None`, an HTTP 500
-		would occur and the diff fragment would appear empty.

Diff:

Revision 2 (+61 -31)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/reviews/views.py

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

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (300dd5b)