Summary

Show commit changes in interdiffs

Review Request #10126 — Created Aug. 24, 2018 and submitted Oct. 18, 2018, 9:39 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~10125~~

Blocks

10127

Commit

25b7f10...

Reviewers

Groups

reviewboard

People

Description

When an interdiff is displayed in the diffviewer, we now show a history
of how the commits changed from the diffset to the interdiffset. The
implementation of diffing between two histories is very basic at the
moment. We currently only directly compare commit IDs as our metric of
whether or not two commits are equivalent. Once any commit IDs differ,
we assume the rest of the commits in the old history will be removed and
the remaining new commits are all added. This is because commit IDs
depend on prior commit IDs in most DVCS systems. Ideally we would use
use commit ancestor information (which is trivally obtainable from
Mercurial and less so from Git).

Testing Done


Ran JS tests.
Ran unit tests.

Files

Issues

Description	From	Last Updated
Can you flesh out the description, describe how it's basic and what's missing?	chipx86	Aug. 27, 2018, 8:15 p.m.
E501 line too long (89 > 79 characters)	reviewbot	Aug. 24, 2018, 8:23 p.m.
Col: 54 Expected a 'break' statement before 'default'.	reviewbot	Aug. 24, 2018, 8:24 p.m.
Col: 13 Unexpected ')'.	reviewbot	Aug. 24, 2018, 8:28 p.m.
Col: 13 Expected an identifier and instead saw ')'.	reviewbot	Aug. 24, 2018, 8:28 p.m.
Col: 17 Expected ')' and instead saw ';'.	reviewbot	Aug. 24, 2018, 8:28 p.m.
Col: 18 Missing semicolon.	reviewbot	Aug. 24, 2018, 8:28 p.m.
Col: 21 It's not necessary to initialize 'rowClass' to 'undefined'.	reviewbot	Aug. 24, 2018, 8:25 p.m.
Col: 17 Unexpected ')'.	reviewbot	Aug. 24, 2018, 8:28 p.m.
Col: 17 Expected an identifier and instead saw ')'.	reviewbot	Aug. 24, 2018, 8:29 p.m.
Col: 21 Expected ')' and instead saw ';'.	reviewbot	Aug. 24, 2018, 8:29 p.m.
Col: 22 Missing semicolon.	reviewbot	Aug. 24, 2018, 8:29 p.m.
Col: 13 Expected an identifier and instead saw ')'.	reviewbot	Aug. 24, 2018, 8:29 p.m.
Col: 13 Unexpected ')'.	reviewbot	Aug. 24, 2018, 8:29 p.m.
Col: 19 Missing semicolon.	reviewbot	Aug. 24, 2018, 8:29 p.m.
Col: 18 Expected ')' and instead saw ';'.	reviewbot	Aug. 24, 2018, 8:29 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Aug. 24, 2018, 8:30 p.m.
This seems unnecessary. Can't we just move that stuff into an __init__ in this class?	chipx86	Aug. 27, 2018, 8:19 p.m.
Needs to be the full type path.	chipx86	Aug. 27, 2018, 8:19 p.m.
Can you pass as keyword arguments, so this is self-describing? Same with all below.	chipx86	Aug. 27, 2018, 8:31 p.m.
Is it really worth having these? The construction is pretty simple.	chipx86	Aug. 27, 2018, 8:19 p.m.
Why aren't we just using __init__? This class is feeling unnecessarily complicated, and I don't have a sense as to …	chipx86	Aug. 27, 2018, 8:19 p.m.
Full type path, here and below.	chipx86	Aug. 27, 2018, 8:19 p.m.
No , or or in these.	chipx86	Aug. 27, 2018, 8:19 p.m.
Same here.	chipx86	Aug. 27, 2018, 8:19 p.m.
Can we say "Iterate through the diff between ...", so it's a bit more clear?	chipx86	Aug. 27, 2018, 8:24 p.m.
No blank line.	chipx86	Aug. 27, 2018, 8:24 p.m.
Doesn't seem like we'd need this namedtuple either. They kinda came out of nowhere, and I'm not sure they're appropriate …	chipx86	Aug. 27, 2018, 8:29 p.m.
This should be a list so that ordering is consistent in the query. That allows the database to cache the …	chipx86	Aug. 27, 2018, 8:30 p.m.
This can easily be one line.	chipx86	Aug. 27, 2018, 8:25 p.m.
These should be on the same line.	chipx86	Aug. 27, 2018, 8:26 p.m.
We have a trailing comma here, but this isn't a .es6.js file. Let's rename it.	chipx86	Aug. 27, 2018, 8:26 p.m.
Blank line between these.	chipx86	Aug. 27, 2018, 8:31 p.m.
Wrong number of spaces for indents.	chipx86	Aug. 27, 2018, 8:32 p.m.
Missing docstring.	chipx86	Aug. 27, 2018, 8:32 p.m.
You can use isEmpty().	chipx86	Aug. 27, 2018, 8:32 p.m.
It's always good to have a break, even in the last case.	chipx86	Aug. 27, 2018, 8:32 p.m.
These can start on the line with the call.	chipx86	Aug. 27, 2018, 8:33 p.m.
Typo: "cellection" -> "collection".	chipx86	Aug. 27, 2018, 8:33 p.m.
F401 'collections.namedtuple' imported but unused	reviewbot	Aug. 27, 2018, 8:49 p.m.
F821 undefined name 'cls'	reviewbot	Aug. 27, 2018, 8:49 p.m.
F821 undefined name 'cls'	reviewbot	Aug. 27, 2018, 8:49 p.m.
F821 undefined name 'cls'	reviewbot	Aug. 27, 2018, 8:49 p.m.
F401 'collections.namedtuple' imported but unused	reviewbot	Aug. 27, 2018, 8:49 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Aug. 27, 2018, 8:49 p.m.
F401 'collections.namedtuple' imported but unused	reviewbot	Aug. 27, 2018, 8:50 p.m.
F821 undefined name 'cls'	reviewbot	Aug. 27, 2018, 8:50 p.m.
F821 undefined name 'cls'	reviewbot	Aug. 27, 2018, 8:50 p.m.
F821 undefined name 'cls'	reviewbot	Aug. 27, 2018, 8:49 p.m.
F401 'collections.namedtuple' imported but unused	reviewbot	Aug. 27, 2018, 8:49 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Aug. 27, 2018, 8:50 p.m.
No trailing period.	chipx86	Aug. 30, 2018, 7:54 p.m.
No trailing period.	chipx86	Aug. 30, 2018, 7:54 p.m.
__init__ doesn't return.	chipx86	Aug. 30, 2018, 8:01 p.m.
Parameters should align.	chipx86	Aug. 30, 2018, 8:02 p.m.
No spaces within {..}	chipx86	Aug. 30, 2018, 8:03 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Aug. 27, 2018, 8:57 p.m.
This doesn't look right.	chipx86	Aug. 30, 2018, 8:03 p.m.
Are i and j ever different? They start as 0, are both incremented at the same time, and never seem …	chipx86	Sept. 4, 2018, 12:53 p.m.
Can you put the full module path? Looks like it'd fit.	chipx86	Sept. 4, 2018, 12:53 p.m.
We always compare the result to the expectation. A failure here with these values backwards could be confusing to debug …	chipx86	Sept. 4, 2018, 12:54 p.m.
""" on the next line.	chipx86	Sept. 4, 2018, 12:54 p.m.
Alphabetical order.	chipx86	Sept. 4, 2018, 12:55 p.m.
Alphabetical order.	chipx86	Sept. 4, 2018, 12:55 p.m.
Alphabetical order.	chipx86	Sept. 4, 2018, 12:55 p.m.
Lookups would be faster if we just have a mapping defined (say, on RB.CommitHistoryDiffEntry, not the prototype) that mapped the …	chipx86	Sept. 4, 2018, 12:58 p.m.
Can you use the expanded form?	chipx86	Sept. 4, 2018, 12:58 p.m.
Same here.	chipx86	Sept. 4, 2018, 12:58 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Aug. 30, 2018, 8:05 p.m.
F821 undefined name 'j'	reviewbot	Sept. 4, 2018, 2:38 p.m.
Col: 2 Missing semicolon.	reviewbot	Sept. 4, 2018, 2:39 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Sept. 4, 2018, 2:38 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Sept. 11, 2018, 2:45 p.m.
Docstring?	david	Sept. 27, 2018, 9:28 a.m.
Docstring?	david	Sept. 27, 2018, 9:29 a.m.
Babel turns this into a monster. We'll need to do it with a bunch of assignments instead.	david	Sept. 27, 2018, 9:30 a.m.
We don't need the $() here, since append() can take an HTML string.	david	Sept. 27, 2018, 9:31 a.m.
Same here with $()	david	Sept. 27, 2018, 9:31 a.m.
And here.	david	Sept. 27, 2018, 9:31 a.m.
E501 line too long (83 > 79 characters)	reviewbot	Sept. 11, 2018, 2:45 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Sept. 27, 2018, 11:01 a.m.
It's best to also provide __ne__ for Python 2.x. On 3.x, we get this for free.	chipx86	Oct. 16, 2018, 12:03 p.m.
Typo: "attribtues" -> "attributes"	chipx86	Oct. 16, 2018, 12:03 p.m.
Kind of confused by the wording here around the double enumerate.	chipx86	Oct. 16, 2018, 12:04 p.m.
Can you wrap this at the "at"? It's slightly too long.	chipx86	Oct. 16, 2018, 12:04 p.m.
We can reduce the generated HTML and the test changes needed if we do: <th<% if (showExpandCollapse) { %>colspan="2"<% } …	chipx86	Oct. 16, 2018, 12:05 p.m.
These are a too long for the lines. How about building the response payload higher up, before the return maybe, …	chipx86	Oct. 16, 2018, 1:03 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Sept. 27, 2018, 11:01 a.m.
Similar to above, let's only add the attribute if we have something other than "1" to set.	chipx86	Oct. 16, 2018, 1:05 p.m.
This isn't a boolean.	chipx86	Oct. 18, 2018, 8:55 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Oct. 18, 2018, 8:55 p.m.

flake8 failed.

JSHint failed.

flake8

reviewboard/reviews/tests/test_builtin_fields.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (89 > 79 characters)
```

JSHint

reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 54
 Expected a 'break' statement before 'default'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 13
 Unexpected ')'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 13
 Expected an identifier and instead saw ')'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 17
 Expected ')' and instead saw ';'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 18
 Missing semicolon.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 21
 It's not necessary to initialize 'rowClass' to 'undefined'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 17
 Unexpected ')'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 17
 Expected an identifier and instead saw ')'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 21
 Expected ')' and instead saw ';'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 22
 Missing semicolon.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 13
 Expected an identifier and instead saw ')'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 13
 Unexpected ')'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 19
 Missing semicolon.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 18
 Expected ')' and instead saw ';'.
```

Change Summary:

jshint

Commit:

-	d4aa747df8a84723f46643e449ba204f0490bca7
+	d7d49e4022cecaaadcc0e833c602462d6b086e70

Diff:

Revision 2 (+666 -90)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	6 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_builtin_fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```

Change Summary:

flake8

Commit:

-	d7d49e4022cecaaadcc0e833c602462d6b086e70
+	833175cae00e9fb725f52a6059ef10f3827780d5

Diff:

Revision 3 (+667 -90)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	6 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Can you flesh out the description, describe how it's basic and what's missing?

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

The issue has been resolved. Show all issues

This seems unnecessary. Can't we just move that stuff into an __init__ in this class?

reviewboard/diffviewer/commit_utils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Needs to be the full type path.
```
reviewboard/diffviewer/commit_utils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you pass as keyword arguments, so this is self-describing?
Same with all below.
```
reviewboard/diffviewer/commit_utils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Is it really worth having these? The construction is pretty simple.
```

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

The issue has been resolved. Show all issues

Why aren't we just using __init__? This class is feeling unnecessarily complicated, and I don't have a sense as to why it needs to be.

reviewboard/diffviewer/commit_utils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Full type path, here and below.
```
reviewboard/diffviewer/commit_utils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
No , or or in these.
```
reviewboard/diffviewer/commit_utils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/diffviewer/commit_utils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we say "Iterate through the diff between ...", so it's a bit more clear?
```
reviewboard/diffviewer/commit_utils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
No blank line.
```

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 3)

The issue has been resolved. Show all issues

Doesn't seem like we'd need this namedtuple either. They kinda came out of nowhere, and I'm not sure they're appropriate for this work.

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

The issue has been resolved. Show all issues

This should be a list so that ordering is consistent in the query. That allows the database to cache the queryset.

reviewboard/diffviewer/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This can easily be one line.
```
reviewboard/diffviewer/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
These should be on the same line.
```
reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.js (Diff revision 3)
The issue has been resolved. Show all issues
```
We have a trailing comma here, but this isn't a .es6.js file. Let's rename it.
```
reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/diffviewer/models/diffCommitListModel.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Wrong number of spaces for indents.
```
reviewboard/static/rb/js/diffviewer/models/diffCommitListModel.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing docstring.
```
reviewboard/static/rb/js/diffviewer/models/diffCommitListModel.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
You can use isEmpty().
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
It's always good to have a break, even in the last case.
```
reviewboard/static/rb/js/pages/models/diffViewerPageModel.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
These can start on the line with the call.
```
reviewboard/static/rb/js/pages/models/diffViewerPageModel.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Typo: "cellection" -> "collection".
```

Change Summary:

Addressed feedback.

Description:

		When an interdiff is displayed in the diffviewer, we now show a history
		of how the commits changed from the diffset to the interdiffset. The
		implementation of diffing between two histories is very basic at the
~		moment.
	~	moment. We currently only directly compare commit IDs as our metric of
	+	whether or not two commits are equivalent. Once any commit IDs differ,
	+	we assume the rest of the commits in the old history will be removed and
	+	the remaining new commits are all added. This is because commit IDs
	+	depend on prior commit IDs in most DVCS systems. Ideally we would use
	+	use commit ancestor information (which is trivally obtainable from
	+	Mercurial and less so from Git).

Commit:

-	833175cae00e9fb725f52a6059ef10f3827780d5
+	e292e377e3ed8798546ce2fe49612c1771839c95

Diff:

Revision 4 (+608 -93)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	6 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/commit_utils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
F401 'collections.namedtuple' imported but unused
```
reviewboard/diffviewer/commit_utils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
F821 undefined name 'cls'
```
reviewboard/diffviewer/commit_utils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
F821 undefined name 'cls'
```
reviewboard/diffviewer/commit_utils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
F821 undefined name 'cls'
```
reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
F401 'collections.namedtuple' imported but unused
```
reviewboard/staticbundles.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E501 line too long (83 > 79 characters)
```

Commit:

-	e292e377e3ed8798546ce2fe49612c1771839c95
+	010afb49c7ff8b44f36263063bd95b9c1b3b8f77

Diff:

Revision 5 (+608 -93)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	6 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/commit_utils.py (Diff revision 5)
The issue has been resolved. Show all issues
```
F401 'collections.namedtuple' imported but unused
```
reviewboard/diffviewer/commit_utils.py (Diff revision 5)
The issue has been resolved. Show all issues
```
F821 undefined name 'cls'
```
reviewboard/diffviewer/commit_utils.py (Diff revision 5)
The issue has been resolved. Show all issues
```
F821 undefined name 'cls'
```
reviewboard/diffviewer/commit_utils.py (Diff revision 5)
The issue has been resolved. Show all issues
```
F821 undefined name 'cls'
```
reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 5)
The issue has been resolved. Show all issues
```
F401 'collections.namedtuple' imported but unused
```
reviewboard/staticbundles.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E501 line too long (83 > 79 characters)
```

Commit:

-	010afb49c7ff8b44f36263063bd95b9c1b3b8f77
+	169d810be8845d28d10a7b482c921ecd485c1c57

Diff:

Revision 6 (+605 -93)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	6 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/staticbundles.py (Diff revision 6)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

reviewboard/diffviewer/commit_utils.py (Diff revision 6)
The issue has been resolved. Show all issues
```
No trailing period.
```
reviewboard/diffviewer/commit_utils.py (Diff revision 6)
The issue has been resolved. Show all issues
```
No trailing period.
```
reviewboard/diffviewer/commit_utils.py (Diff revision 6)
The issue has been resolved. Show all issues
```
__init__ doesn't return.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Parameters should align.
```
reviewboard/static/rb/js/diffviewer/views/tests/diffCommitListViewTests.es6.js (Diff revision 6)
The issue has been resolved. Show all issues
```
No spaces within {..}
```
reviewboard/staticbundles.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This doesn't look right.
```

Change Summary:

Addressed feedback

Commit:

-	169d810be8845d28d10a7b482c921ecd485c1c57
+	7e488185aba379d628598723d5d12b86132bc950

Diff:

Revision 7 (+675 -96)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	6 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/staticbundles.py (Diff revision 7)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

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

The issue has been resolved. Show all issues

Are i and j ever different? They start as 0, are both incremented at the same time, and never seem to otherwise change.

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Can you put the full module path? Looks like it'd fit.
```

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 7)

The issue has been resolved. Show all issues

We always compare the result to the expectation. A failure here with these values backwards could be confusing to debug at first. These should all be fixed to follow the typical argument order.

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 7)
The issue has been resolved. Show all issues
```
""" on the next line.
```

reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js (Diff revision 7)

The issue has been resolved. Show all issues

Alphabetical order.

reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Alphabetical order.
```
reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Alphabetical order.
```

reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js (Diff revision 7)

The issue has been resolved. Show all issues

Lookups would be faster if we just have a mapping defined (say, on RB.CommitHistoryDiffEntry, not the prototype) that mapped the entryType values to the symbols. Lookups are faster, and getSymbol() could just wrap this.

reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Can you use the expanded form?
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Same here.
```

Change Summary:

Addressed feedback

Commit:

-	7e488185aba379d628598723d5d12b86132bc950
+	c31a8cded7083aa379529eea98e700381da77f80

Diff:

Revision 8 (+669 -85)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	6 more

Checks run (2 failed)

flake8 failed.

JSHint failed.

flake8

reviewboard/diffviewer/commit_utils.py (Diff revision 8)
The issue has been resolved. Show all issues
```
F821 undefined name 'j'
```
reviewboard/staticbundles.py (Diff revision 8)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

JSHint

reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 2
 Missing semicolon.
```

Change Summary:

Fixes from accidental publish

Commit:

-	c31a8cded7083aa379529eea98e700381da77f80
+	a1163880fc94cce4b4dad15a54b2400c7e110aed

Diff:

Revision 9 (+674 -85)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	6 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/staticbundles.py (Diff revision 9)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

Change Summary:

Unit test changes from /r/10125/

Commit:

-	a1163880fc94cce4b4dad15a54b2400c7e110aed
+	f1885c258e7e68b106525cee3ff5db86385970f6

Diff:

Revision 10 (+719 -91)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	7 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/staticbundles.py (Diff revision 10)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

reviewboard/diffviewer/commit_utils.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Docstring?
```
reviewboard/diffviewer/commit_utils.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Docstring?
```

reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js (Diff revision 10)

The issue has been resolved. Show all issues

Babel turns this into a monster. We'll need to do it with a bunch of assignments instead.

brennie Sept. 27, 2018, 9:28 a.m.
```
:((((
```

reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 10)
The issue has been resolved. Show all issues
```
We don't need the $() here, since append() can take an HTML string.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 10)
The issue has been resolved. Show all issues
```
Same here with $()
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 10)
The issue has been resolved. Show all issues
```
And here.
```

Change Summary:

Addressed feedback.

Commit:

-	f1885c258e7e68b106525cee3ff5db86385970f6
+	9ab5d747cbd915338cf92fcb942d2ed77e92b06a

Diff:

Revision 11 (+737 -91)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	7 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/staticbundles.py (Diff revision 11)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

Change Summary:

Fix unit test

Commit:

-	9ab5d747cbd915338cf92fcb942d2ed77e92b06a
+	3dc0db202fecbba4a5aa4069bd385712396f5a5e

Diff:

Revision 12 (+744 -91)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	7 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/staticbundles.py (Diff revision 12)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

reviewboard/diffviewer/commit_utils.py (Diff revision 12)
The issue has been resolved. Show all issues
```
It's best to also provide __ne__ for Python 2.x. On 3.x, we get this for free.
```
reviewboard/diffviewer/commit_utils.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Typo: "attribtues" -> "attributes"
```
reviewboard/diffviewer/commit_utils.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Kind of confused by the wording here around the double enumerate.
```
1. brennie Oct. 16, 2018, 1:05 p.m.
  I think it got garbled while reflowing it, oops
reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Can you wrap this at the "at"? It's slightly too long.
```

reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 12)

The issue has been resolved. Show all issues

We can reduce the generated HTML and the test changes needed if we do:

<th<% if (showExpandCollapse) { %>colspan="2"<% } %>>

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

The issue has been resolved. Show all issues

These are a too long for the lines. How about building the response payload higher up, before the return maybe, and just doing cb(responsePayload) or something.

reviewboard/templates/reviews/commit_list_field.html (Diff revision 12)
The issue has been resolved. Show all issues
```
Similar to above, let's only add the attribute if we have something other than "1" to set.
```

Change Summary:

Addressed feedback.

Commit:

-	3dc0db202fecbba4a5aa4069bd385712396f5a5e
+	25b7f10ec2b345ad34876050ec08c19803ffd79e

Diff:

Revision 13 (+778 -104)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js
	reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js
	7 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/staticbundles.py (Diff revision 13)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

Ship it!

reviewboard/static/rb/js/diffviewer/models/diffCommitListModel.es6.js (Diff revision 13)
The issue has been resolved. Show all issues
```
This isn't a boolean.
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (76f671a)