Summary:

Switch between selected commit revisions shown in the diffviewer

Review Request #10136 — Created Sept. 10, 2018 and submitted Oct. 18, 2018, 9:39 p.m.

Information
Owner:	brennie
Repository:	Review Board
Branch:	release-4.0.x
Bugs:
Depends On:	~~10132~~, ~~10231~~, ~~10186~~
Blocks:	10190, 10233, 10191
Commit:	d543ed9...
Reviewers
Groups:	reviewboard
People:

Description

This patch builds upon the UI in /r/10132/ and implements the
functionality to render the actual content of the diff specified by the
selected commit range.

Testing Done


Ran unit tests.
Ran JS tests.
Viewed the diffs generated -- they looked correct.

Issues

Description	From	Last Updated
Would it be possible to split out the ES6 port part of this change into a separate review request?	david	Sept. 27, 2018, 10:49 a.m.
W391 blank line at end of file	reviewbot	Sept. 10, 2018, 3:03 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Sept. 10, 2018, 3:04 p.m.
E501 line too long (90 > 79 characters)	reviewbot	Sept. 10, 2018, 3:04 p.m.
W391 blank line at end of file	reviewbot	Sept. 11, 2018, 8:02 p.m.
F841 local variable 'diffset_commits' is assigned to but never used	reviewbot	Sept. 11, 2018, 8:02 p.m.
Col: 10 Missing semicolon.	reviewbot	Sept. 11, 2018, 8:02 p.m.
Col: 70 Missing semicolon.	reviewbot	Sept. 11, 2018, 8:02 p.m.
Col: 69 Missing semicolon.	reviewbot	Sept. 11, 2018, 8:03 p.m.
request -> requested?	david	Sept. 27, 2018, 10:50 a.m.
request -> requested?	david	Sept. 27, 2018, 10:50 a.m.
There's a lot of nesting and conditionals in here. Can we split this up to be more readable?	david	Sept. 27, 2018, 10:53 a.m.
Too many blank lines now.	david	Sept. 27, 2018, 10:54 a.m.
Needs a trailing comma.	david	Sept. 27, 2018, 10:58 a.m.
Needs a trailing comma.	david	Sept. 27, 2018, 10:58 a.m.
Missing the explanations here.	david	Sept. 27, 2018, 10:55 a.m.
Needs a doc comment.	david	Sept. 27, 2018, 10:57 a.m.
Looks to me like this returns a function, not an object.	david	Sept. 27, 2018, 10:58 a.m.
Can we use a ~ for each class reference to trim the output a bit?	chipx86	Oct. 16, 2018, 2 p.m.
Let's get rid of the double loop and need for constructing generators. What do you think of: needs_base_commit = base_commit_id …	chipx86	Oct. 16, 2018, 2:15 p.m.
Too many blank lines.	chipx86	Oct. 16, 2018, 2:04 p.m.
There's some redundancy in handling of missing data here. We know index is in GET, but we call .get() on …	chipx86	Oct. 16, 2018, 2:06 p.m.
string	chipx86	Oct. 16, 2018, 2:06 p.m.
string	chipx86	Oct. 16, 2018, 2:15 p.m.
Can you put parens around the expression?	chipx86	Oct. 16, 2018, 2:07 p.m.
These are indented too far.	chipx86	Oct. 16, 2018, 2:07 p.m.
These should all align.	chipx86	Oct. 18, 2018, 9:08 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/commit_utils.py (Diff revision 1)
Show all issues
```
W391 blank line at end of file
```
reviewboard/diffviewer/views.py (Diff revision 1)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/reviews/views.py (Diff revision 1)
Show all issues
```
E501 line too long (90 > 79 characters)
```

Change Summary:

Rebased. Added js tests.

Summary:

-	WIP: Switch between selected commit revisions shown in the diffviewer
+	Switch between selected commit revisions shown in the diffviewer

Commit:

-	95605536806a8870ae11a35498bacb3a4f17d1d7
+	1eb2d33897c22087d203da658338f67cfc28b49b

Diff:

Revision 2 (+1130 -236)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.es6.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffReviewableCollectionTests.es6.js
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.js
	11 more

Checks run (2 failed)

flake8 failed.

JSHint failed.

flake8

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 2)
Show all issues
```
W391 blank line at end of file
```

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

Show all issues

F841 local variable 'diffset_commits' is assigned to but never used

JSHint

reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 2)
Show all issues
```
Col: 10
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 2)
Show all issues
```
Col: 70
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 2)
Show all issues
```
Col: 69
 Missing semicolon.
```

Change Summary:

jshint,pep8

Commit:

-	1eb2d33897c22087d203da658338f67cfc28b49b
+	a9692dab919775e59bf2bfac27d85cfdb8ae85be

Diff:

Revision 3 (+1127 -236)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.es6.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffReviewableCollectionTests.es6.js
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.js
	11 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Show all issues

Would it be possible to split out the ES6 port part of this change into a separate review request?

brennie Sept. 27, 2018, 10:49 a.m.

That is now https://reviews.reviewboard.org/r/10186/.

reviewboard/diffviewer/commit_utils.py (Diff revision 3)
Show all issues
```
request -> requested?
```
reviewboard/diffviewer/commit_utils.py (Diff revision 3)
Show all issues
```
request -> requested?
```

reviewboard/diffviewer/commit_utils.py (Diff revision 3)

Show all issues

There's a lot of nesting and conditionals in here.

Can we split this up to be more readable?

reviewboard/reviews/views.py (Diff revision 3)
Show all issues
```
Too many blank lines now.
```
reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js (Diff revision 3)
Show all issues
```
Needs a trailing comma.
```
reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js (Diff revision 3)
Show all issues
```
Needs a trailing comma.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3)
Show all issues
```
Missing the explanations here.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3)
Show all issues
```
Needs a doc comment.
```
reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 3)
Show all issues
```
Looks to me like this returns a function, not an object.
```

Depends On:

+	~~10186 - Port some JS models and tests to ES6~~

Commit:

-	a9692dab919775e59bf2bfac27d85cfdb8ae85be
+	b697192e1d3627ff2cf6c226fe52579f0768f3fe

Diff:

Revision 4 (+946 -132)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.es6.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffReviewableCollectionTests.es6.js
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	6 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	b697192e1d3627ff2cf6c226fe52579f0768f3fe
+	7158966c0cb57dbd306dcb9ee22499bfaed0c385

Diff:

Revision 5 (+967 -132)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.es6.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffReviewableCollectionTests.es6.js
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	6 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Adds missing unit tests

Depends On:

+	~~10231 - Remove erronously introduced tests~~

Commit:

-	7158966c0cb57dbd306dcb9ee22499bfaed0c385
+	3b1c31d97d42a0d50ae60acc91c32afe5ea2f9a9

Diff:

Revision 6 (+990 -132)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.es6.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffReviewableCollectionTests.es6.js
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	7 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/commit_utils.py (Diff revision 6)

Show all issues

Can we use a ~ for each class reference to trim the output a bit?

reviewboard/diffviewer/commit_utils.py (Diff revision 6)

Show all issues

Let's get rid of the double loop and need for constructing generators. What do you think of:

needs_base_commit = base_commit_id is not None
needs_tip_commit = tip_commit_id is not None

if needs_base_commit or needs_tip_commit:
    for commit in commits:
        if needs_base_commit and commit.pk == base_commit_id:
            base_commit = commit
            needs_base_commit = False

        if needs_tip_commit and commit.pk == tip_commit_id:
            tip_commit = commit
            needs_tip_commit = False

        if not needs_base_commit and not needs_tip_commit:
            break

brennie Oct. 16, 2018, 2:15 p.m.

I think it can be more simplified since (a) commit PKs will never be None so we can always just jump to the comparison and (b) keeping the needs_{base|tip}_commit seems unnecessary since at most there are two commits we need to iterate over.

reviewboard/diffviewer/views.py (Diff revision 6)
Show all issues
```
Too many blank lines.
```

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

Show all issues

There's some redundancy in handling of missing data here. We know index is in GET, but we call .get() on it anyways. We're also then doing exception handling anywya. How about instead we do:

try:
    diff_file['index'] = int(self.request.GET['index'])
except (KeyError, ValueError):
    pass

reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.es6.js (Diff revision 6)
Show all issues
```
string
```
reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.es6.js (Diff revision 6)
Show all issues
```
string
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 6)
Show all issues
```
Can you put parens around the expression?
```

reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 6)

Show all issues

These are indented too far.

Change Summary:

Addressed feedback.

Commit:

-	3b1c31d97d42a0d50ae60acc91c32afe5ea2f9a9
+	d543ed966b0a1ac50afd49cebfbc67c1753a1bb5

Diff:

Revision 7 (+958 -106)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.es6.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffReviewableCollectionTests.es6.js
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	7 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 7)
Show all issues
```
These should all align.
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (1f60295)