Summary

Add cumulative diff API

Review Request #10233 — Created Oct. 15, 2018 and submitted Oct. 25, 2018, 10:28 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~10136~~, ~~10216~~

Blocks

10240, 10245

Commit

2edb57f...

Reviewers

Groups

reviewboard

People

Description

Previously, we uploaded only per-commit FileDiffs to review requests
with history support. Now, we also require API clients to upload a
"cumulative diff" -- that is, a diff that represents the squashed commit
range -- to the draft diff API. This also results in the "finalization"
of the commit series, which prevents uploading of additional commits, as
this would result in a cumulative diff that does not represent the
entire uploaded range.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
F401 'django.utils.functional.cached_property' imported but unused	reviewbot	Oct. 18, 2018, 5:40 p.m.
F841 local variable 'commit' is assigned to but never used	reviewbot	Oct. 18, 2018, 5:40 p.m.
F841 local variable 'commit' is assigned to but never used	reviewbot	Oct. 18, 2018, 5:40 p.m.
F401 'reviewboard.reviews.signals.review_request_published' imported but unused	reviewbot	Oct. 18, 2018, 5:40 p.m.
F401 'djblets.webapi.fields.BooleanFieldType' imported but unused	reviewbot	Oct. 18, 2018, 5:40 p.m.
F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused	reviewbot	Oct. 18, 2018, 5:40 p.m.
E131 continuation line unaligned for hanging indent	reviewbot	Oct. 18, 2018, 5:40 p.m.
F811 redefinition of unused 'test_put_finalize_invalid_validation_info_json_format' from line 649	reviewbot	Oct. 18, 2018, 5:40 p.m.
E303 too many blank lines (2)	reviewbot	Oct. 18, 2018, 5:40 p.m.
F821 undefined name 'commit'	reviewbot	Oct. 18, 2018, 5:40 p.m.
F821 undefined name 'commit'	reviewbot	Oct. 18, 2018, 5:40 p.m.
W391 blank line at end of file	reviewbot	Oct. 18, 2018, 5:40 p.m.
F401 'django.utils.functional.cached_property' imported but unused	reviewbot	Oct. 18, 2018, 5:41 p.m.
F841 local variable 'commit' is assigned to but never used	reviewbot	Oct. 18, 2018, 5:41 p.m.
F841 local variable 'commit' is assigned to but never used	reviewbot	Oct. 18, 2018, 5:41 p.m.
F401 'reviewboard.diffviewer.models.DiffSet' imported but unused	reviewbot	Oct. 18, 2018, 5:41 p.m.
F401 'reviewboard.reviews.signals.review_request_published' imported but unused	reviewbot	Oct. 18, 2018, 5:42 p.m.
F401 'djblets.webapi.fields.BooleanFieldType' imported but unused	reviewbot	Oct. 18, 2018, 5:42 p.m.
F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused	reviewbot	Oct. 18, 2018, 5:42 p.m.
E131 continuation line unaligned for hanging indent	reviewbot	Oct. 18, 2018, 5:43 p.m.
F811 redefinition of unused 'test_put_finalize_invalid_validation_info_json_format' from line 649	reviewbot	Oct. 18, 2018, 5:43 p.m.
E303 too many blank lines (2)	reviewbot	Oct. 18, 2018, 5:43 p.m.
E303 too many blank lines (2)	reviewbot	Oct. 18, 2018, 5:44 p.m.
F841 local variable 'rsp' is assigned to but never used	reviewbot	Oct. 18, 2018, 5:44 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Oct. 18, 2018, 8:37 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Oct. 18, 2018, 8:37 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Oct. 18, 2018, 8:37 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Oct. 18, 2018, 8:37 p.m.
This is to prevent interdiffs when viewing a subset of commits, right? Can you add a comment and assertion string …	chipx86	Oct. 18, 2018, 10:09 p.m.
This can be simplified a bit (might as well bail early if self.extra_data is empty): return (self.extra_data and self.extra_data.get(self._FINALIZED_COMMIT_SERIES_KEY, False))	chipx86	Oct. 18, 2018, 10:43 p.m.
Comma after True.	chipx86	Oct. 18, 2018, 10:10 p.m.
Remove one of the "been"s.	chipx86	Oct. 18, 2018, 10:10 p.m.
Can you change "incurred" to "performed"? Seems more appropriate.	chipx86	Oct. 18, 2018, 10:11 p.m.
Is the first implementation Django 1.6, and the other 1.8+? If so, can you tag with: if blah: # Django …	chipx86	Oct. 18, 2018, 10:11 p.m.
related	chipx86	Oct. 18, 2018, 10:11 p.m.
per_commit_files	chipx86	Oct. 18, 2018, 10:11 p.m.
objects	chipx86	Oct. 18, 2018, 10:12 p.m.
This should be moved in one level, right?	chipx86	Oct. 18, 2018, 10:12 p.m.
Swap these.	chipx86	Oct. 18, 2018, 10:13 p.m.
Can you change this to use the standard styling? for ... should be on its own line in each case.	chipx86	Oct. 18, 2018, 10:13 p.m.
Only one blank line.	chipx86	Oct. 18, 2018, 10:13 p.m.
Missing ugettext.	chipx86	Oct. 18, 2018, 10:13 p.m.
Two newlines. (One on the next line, to help with readability.)	chipx86	Oct. 18, 2018, 10:14 p.m.
Double backticks for the field names, and for true (which should be lowercase, since it's not Python's version).	chipx86	Oct. 18, 2018, 10:15 p.m.
Typo: "cumulative"	chipx86	Oct. 18, 2018, 10:15 p.m.
Missing API docs.	chipx86	Oct. 18, 2018, 10:46 p.m.
We should be saying "commit history support".	chipx86	Oct. 18, 2018, 10:16 p.m.
I feel like we should be moving this out into a proper method somewhere that can be called on DiffSet. …	chipx86	Oct. 23, 2018, 3:34 p.m.
The backtick is in the wrong place.	chipx86	Oct. 18, 2018, 10:16 p.m.
No blank line.	chipx86	Oct. 18, 2018, 10:16 p.m.
Let's use .exists(). It can be optimized better.	chipx86	Oct. 18, 2018, 10:16 p.m.
Yes, it should be. We use DOES_NOT_EXIST for resources that don't exist.	chipx86	Oct. 18, 2018, 10:19 p.m.
We quote the field name here, but not in the other errors above or below.	chipx86	Oct. 18, 2018, 10:19 p.m.
Can you expand this into the standard multi-line format?	chipx86	Oct. 18, 2018, 10:19 p.m.
Alphabetical order.	chipx86	Oct. 18, 2018, 10:19 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Oct. 18, 2018, 8:37 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Oct. 18, 2018, 8:37 p.m.
F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused	reviewbot	Oct. 23, 2018, 2:26 p.m.
F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused	reviewbot	Oct. 23, 2018, 2:26 p.m.
F401 'django.utils.six' imported but unused	reviewbot	Oct. 23, 2018, 5:57 p.m.
F841 local variable 'errors_by_code' is assigned to but never used	reviewbot	Oct. 23, 2018, 5:57 p.m.
Can you indent the right-hand side of the != one level?	chipx86	Oct. 24, 2018, 2:43 p.m.
"commit history"	chipx86	Oct. 24, 2018, 2:43 p.m.
"commit history"	chipx86	Oct. 24, 2018, 2:43 p.m.
"commit history"	chipx86	Oct. 24, 2018, 2:43 p.m.
Probably best as an itemized list.	chipx86	Oct. 24, 2018, 2:48 p.m.
This could probably just be a single line.	chipx86	Oct. 24, 2018, 2:46 p.m.
"commit history"	chipx86	Oct. 24, 2018, 2:46 p.m.
"commit history"	chipx86	Oct. 24, 2018, 2:47 p.m.
"commit history"	chipx86	Oct. 24, 2018, 2:47 p.m.
Let's add a blank line between these.	chipx86	Oct. 25, 2018, 9:46 p.m.
The second line should be aligned with the first.	chipx86	Oct. 25, 2018, 9:46 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/models/diffset.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'django.utils.functional.cached_property' imported but unused
```
reviewboard/diffviewer/tests/test_diffset.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F841 local variable 'commit' is assigned to but never used
```
reviewboard/diffviewer/tests/test_diffset.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F841 local variable 'commit' is assigned to but never used
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'reviewboard.reviews.signals.review_request_published' imported but unused
```
reviewboard/webapi/resources/diff.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'djblets.webapi.fields.BooleanFieldType' imported but unused
```
reviewboard/webapi/resources/diff.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E131 continuation line unaligned for hanging indent
```

reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)

The issue has been dropped. Show all issues

F811 redefinition of unused 'test_put_finalize_invalid_validation_info_json_format' from line 649

reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F821 undefined name 'commit'
```
reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F821 undefined name 'commit'
```
reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)
The issue has been dropped. Show all issues
```
W391 blank line at end of file
```

Summary:

-	WIP: Commit Series Finalization API
+	Add cumulative diff API

Description:

~		WIP
	~	Previously, we uploaded only per-commit FileDiffs to review requests
	+	with history support. Now, we also require API clients to upload a
	+	"cumulative diff" -- that is, a diff that represents the squashed commit
	+	range -- to the draft diff API. This also results in the "finalization"
	+	of the commit series, which prevents uploading of additional commits, as
	+	this would result in a cumulative diff that does not represent the
	+	entire uploaded range.

Testing Done:

Ran unit tests.

Depends On:

-	~~10232 - Split filediff creation into preparation and finalization methods~~
+	~~10216 - Remove DiffCommit.file_count and DiffSet.file_count fields~~
+	~~10136 - Switch between selected commit revisions shown in the diffviewer~~

Commit:

-	321daa834786320d19ffa9ac7638ddc57004d5dd
+	5bf70826895fcffb72c9525db1ae0bad8b52a505

Diff:

Revision 2 (+1232 -89)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/reviews/tests/test_review_request_draft.py
	7 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/models/diffset.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F401 'django.utils.functional.cached_property' imported but unused
```
reviewboard/diffviewer/tests/test_diffset.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'commit' is assigned to but never used
```
reviewboard/diffviewer/tests/test_diffset.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'commit' is assigned to but never used
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F401 'reviewboard.diffviewer.models.DiffSet' imported but unused
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F401 'reviewboard.reviews.signals.review_request_published' imported but unused
```
reviewboard/webapi/resources/diff.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F401 'djblets.webapi.fields.BooleanFieldType' imported but unused
```
reviewboard/webapi/resources/diff.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E131 continuation line unaligned for hanging indent
```

reviewboard/webapi/tests/test_draft_diff.py (Diff revision 2)

The issue has been resolved. Show all issues

F811 redefinition of unused 'test_put_finalize_invalid_validation_info_json_format' from line 649

reviewboard/webapi/tests/test_draft_diff.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'rsp' is assigned to but never used
```

Change Summary:

flake8 fixups

Commit:

-	5bf70826895fcffb72c9525db1ae0bad8b52a505
+	6709b87ed7321e4c90b22e9333678f98c5af6ee8

Diff:

Revision 3 (+1225 -87)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/reviews/tests/test_review_request_draft.py
	7 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Add unit test

Commit:

-	6709b87ed7321e4c90b22e9333678f98c5af6ee8
+	5121c1e069d30aeefc91a513756a66eefb346e67

Diff:

Revision 4 (+1314 -130)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/reviews/tests/test_review_request_draft.py
	9 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

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

Commit:

-	5121c1e069d30aeefc91a513756a66eefb346e67
+	cfbc9b1ac4cc3ab067add2ba048b30ae16e13067

Diff:

Revision 5 (+1313 -131)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/reviews/tests/test_review_request_draft.py
	9 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

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

Commit:

-	cfbc9b1ac4cc3ab067add2ba048b30ae16e13067
+	e508d8e71d04cc610ff8364bc4b2ce8de4c78b82

Diff:

Revision 6 (+1318 -131)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/reviews/tests/test_review_request_draft.py
	9 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

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

Commit:

-	e508d8e71d04cc610ff8364bc4b2ce8de4c78b82
+	a3a4a6d051b50b1e1c3f6475008c1db448de6861

Diff:

Revision 7 (+1320 -135)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

This is to prevent interdiffs when viewing a subset of commits, right? Can you add a comment and assertion string describing this? It'll help if we need to change it later.

reviewboard/diffviewer/models/diffset.py (Diff revision 6)

The issue has been resolved. Show all issues

This can be simplified a bit (might as well bail early if self.extra_data is empty):

return (self.extra_data and
        self.extra_data.get(self._FINALIZED_COMMIT_SERIES_KEY, False))

reviewboard/diffviewer/models/diffset.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Comma after True.
```
reviewboard/diffviewer/models/diffset.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Remove one of the "been"s.
```
reviewboard/diffviewer/models/diffset.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Can you change "incurred" to "performed"? Seems more appropriate.
```

reviewboard/diffviewer/models/diffset.py (Diff revision 6)

The issue has been resolved. Show all issues

Is the first implementation Django 1.6, and the other 1.8+? If so, can you tag with:

if blah:
    # Django == 1.6
else:
    # Django >= 1.8

Same further down.

reviewboard/diffviewer/models/diffset.py (Diff revision 6)
The issue has been resolved. Show all issues
```
related
```
reviewboard/diffviewer/models/diffset.py (Diff revision 6)
The issue has been resolved. Show all issues
```
per_commit_files
```
reviewboard/diffviewer/models/diffset.py (Diff revision 6)
The issue has been resolved. Show all issues
```
objects
```
reviewboard/diffviewer/models/diffset.py (Diff revision 6)
The issue has been dropped. Show all issues
```
This should be moved in one level, right?
```
1. brennie Oct. 18, 2018, 10:12 p.m.
  Why? It doesn't matter.
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Swap these.
```

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 6)

The issue has been resolved. Show all issues

Can you change this to use the standard styling? for ... should be on its own line in each case.

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Only one blank line.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Missing ugettext.
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Two newlines. (One on the next line, to help with readability.)
```

reviewboard/webapi/resources/draft_diff.py (Diff revision 6)

The issue has been resolved. Show all issues

Double backticks for the field names, and for true (which should be lowercase, since it's not Python's version).

reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Typo: "cumulative"
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Missing API docs.
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
We should be saying "commit history support".
```

reviewboard/webapi/resources/draft_diff.py (Diff revision 6)

The issue has been resolved. Show all issues

I feel like we should be moving this out into a proper method somewhere that can be called on DiffSet. The reason being that for pull request support, we're going to need to be able to finalize commits. Ideally, the REST API would be a somewhat-thin wrapper around a proper DiffSet API.

brennie Oct. 18, 2018, 10:32 p.m.

Ive ripped out everything except the validation portion. I dont know that it feels correct to raise ValidationErrors in a non-form context. Should this be a form?

chipx86 Oct. 23, 2018, 2:33 p.m.

forms re-imports ValidationError for historical reasons/convenience, but it's a core-level exception in Django. It's okay to raise them if it's validation-related.

reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
The backtick is in the wrong place.
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
No blank line.
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Let's use .exists(). It can be optimized better.
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Yes, it should be. We use DOES_NOT_EXIST for resources that don't exist.
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
We quote the field name here, but not in the other errors above or below.
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Can you expand this into the standard multi-line format?
```
reviewboard/webapi/tests/test_draft_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Alphabetical order.
```

Change Summary:

Addressed feedback

Commit:

-	a3a4a6d051b50b1e1c3f6475008c1db448de6861
+	3ebed35de6dad52c167a1b89cfc2153dd4874496

Diff:

Revision 8 (+1396 -135)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	10 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 8)
The issue has been resolved. Show all issues
```
F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 8)
The issue has been resolved. Show all issues
```
F401 'reviewboard.diffviewer.filediff_creator.create_filediffs' imported but unused
```

Commit:

-	3ebed35de6dad52c167a1b89cfc2153dd4874496
+	12b7fa6bc63483def424883201a38a8b216cb183

Diff:

Revision 9 (+1394 -135)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Addressed feedback

Commit:

-	12b7fa6bc63483def424883201a38a8b216cb183
+	99fbc67042cc844538158dfd45346d21d564387c

Diff:

Revision 10 (+1425 -138)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	10 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/draft_diff.py (Diff revision 10)
The issue has been resolved. Show all issues
```
F401 'django.utils.six' imported but unused
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 10)
The issue has been resolved. Show all issues
```
F841 local variable 'errors_by_code' is assigned to but never used
```

Change Summary:

Flake8 fixups

Commit:

-	99fbc67042cc844538158dfd45346d21d564387c
+	6efca49b8c3e9c43814ffa893b8c3343275e6156

Diff:

Revision 11 (+1419 -138)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/models/diffset.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Can you indent the right-hand side of the != one level?
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 11)
The issue has been resolved. Show all issues
```
"commit history"
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 11)
The issue has been resolved. Show all issues
```
"commit history"
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 11)
The issue has been resolved. Show all issues
```
"commit history"
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Probably best as an itemized list.
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 11)
The issue has been resolved. Show all issues
```
This could probably just be a single line.
```
reviewboard/webapi/tests/test_draft_diff.py (Diff revision 11)
The issue has been resolved. Show all issues
```
"commit history"
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 11)
The issue has been resolved. Show all issues
```
"commit history"
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 11)
The issue has been resolved. Show all issues
```
"commit history"
```

Change Summary:

Addressed feedback

Commit:

-	6efca49b8c3e9c43814ffa893b8c3343275e6156
+	2f12cbce3a6dec814846b1881da3d8cc3b23216a

Diff:

Revision 12 (+1417 -138)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/webapi/resources/draft_diff.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Let's add a blank line between these.
```
reviewboard/webapi/resources/draft_diff.py (Diff revision 12)
The issue has been resolved. Show all issues
```
The second line should be aligned with the first.
```

Change Summary:

Addressed feedback.

Commit:

-	2f12cbce3a6dec814846b1881da3d8cc3b23216a
+	2edb57f93a53d8a2cd09595d5f9d1b685e468bc5

Diff:

Revision 13 (+1418 -138)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/tests/test_diffset.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_builtin_fields.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (725c597)