brennie got a fish trophy!

Summary

Add resources for interacting with DiffCommits

Review Request #9889 — Created May 2, 2018 and submitted Aug. 28, 2018, 8:40 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~9883~~

Blocks

9905, 10118

Commit

3118383...

Reviewers

Groups

reviewboard

People

Description

The DraftDiffCommitResource allows new commits to be uploaded to a
review request draft with history support and the DiffCommitResource
allows fetching information about created commits.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
E501 line too long (81 > 79 characters)	reviewbot	May 2, 2018, 10:21 p.m.
F401 'django.utils.six' imported but unused	reviewbot	May 2, 2018, 10:21 p.m.
F821 undefined name 'diff_commit'	reviewbot	May 2, 2018, 10:21 p.m.
F821 undefined name 'diff_commit'	reviewbot	May 2, 2018, 10:21 p.m.
F821 undefined name 'diff_commit'	reviewbot	May 2, 2018, 10:21 p.m.
F841 local variable 'review_request' is assigned to but never used	reviewbot	May 2, 2018, 10:22 p.m.
E303 too many blank lines (2)	reviewbot	May 2, 2018, 10:22 p.m.
E231 missing whitespace after ','	reviewbot	May 2, 2018, 10:22 p.m.
F841 local variable 'commit' is assigned to but never used	reviewbot	May 2, 2018, 10:23 p.m.
E303 too many blank lines (2)	reviewbot	May 2, 2018, 10:23 p.m.
F841 local variable 'draft' is assigned to but never used	reviewbot	May 2, 2018, 10:24 p.m.
F841 local variable 'draft' is assigned to but never used	reviewbot	May 2, 2018, 10:24 p.m.
F811 redefinition of unused 'test_get_links_dvcs_enabled' from line 268	reviewbot	May 2, 2018, 10:24 p.m.
F841 local variable 'draft' is assigned to but never used	reviewbot	May 2, 2018, 10:24 p.m.
E302 expected 2 blank lines, found 1	reviewbot	May 2, 2018, 10:25 p.m.
E501 line too long (81 > 79 characters)	reviewbot	May 2, 2018, 10:35 p.m.
F401 'django.utils.six' imported but unused	reviewbot	May 2, 2018, 10:35 p.m.
F821 undefined name 'diff_commit'	reviewbot	May 2, 2018, 10:35 p.m.
F821 undefined name 'diff_commit'	reviewbot	May 2, 2018, 10:35 p.m.
F821 undefined name 'diff_commit'	reviewbot	May 2, 2018, 10:35 p.m.
F841 local variable 'review_request' is assigned to but never used	reviewbot	May 2, 2018, 10:35 p.m.
E303 too many blank lines (2)	reviewbot	May 2, 2018, 10:35 p.m.
E231 missing whitespace after ','	reviewbot	May 2, 2018, 10:35 p.m.
F841 local variable 'commit' is assigned to but never used	reviewbot	May 2, 2018, 10:35 p.m.
E303 too many blank lines (2)	reviewbot	May 2, 2018, 10:35 p.m.
F841 local variable 'draft' is assigned to but never used	reviewbot	May 2, 2018, 10:35 p.m.
F841 local variable 'draft' is assigned to but never used	reviewbot	May 2, 2018, 10:35 p.m.
F811 redefinition of unused 'test_get_links_dvcs_enabled' from line 268	reviewbot	May 2, 2018, 10:35 p.m.
F841 local variable 'draft' is assigned to but never used	reviewbot	May 2, 2018, 10:35 p.m.
E302 expected 2 blank lines, found 1	reviewbot	May 2, 2018, 10:35 p.m.
F401 'reviewboard.diffviewer.models.DiffSet' imported but unused	reviewbot	May 4, 2018, 9:30 p.m.
E303 too many blank lines (2)	reviewbot	May 4, 2018, 10:45 p.m.
This isn't in the function signature.	chipx86	July 4, 2018, 11:19 p.m.
Missing a docstring.	chipx86	July 4, 2018, 11:20 p.m.
Too many blank lines.	chipx86	July 4, 2018, 11:21 p.m.
The and .. or form was used in the past due to lack of the inline conditional support in earlier …	chipx86	July 4, 2018, 11:47 p.m.
"ID"	chipx86	July 4, 2018, 11:21 p.m.
We shouldn't use literals in summaries. Same below.	chipx86	July 4, 2018, 11:21 p.m.
You can just use ObjectDoesNotExist here.	chipx86	July 4, 2018, 11:22 p.m.
rsp	chipx86	July 4, 2018, 11:22 p.m.
"solely"	chipx86	July 4, 2018, 11:22 p.m.
Everything is indented 1 space in the body.	chipx86	July 4, 2018, 11:23 p.m.
Why can't we just tack on ?commit-id=... onto the URL? Are we dealing with dynamic content in the link that …	chipx86	July 4, 2018, 11:25 p.m.
Let's explicitly compare obj against None, since the former's gotten us into trouble before with valid but falsy-evaluating objects.	chipx86	July 4, 2018, 11:25 p.m.
Can you pass these params as keyword arguments, to help with readability?	chipx86	July 4, 2018, 11:27 p.m.
Trailing comma is turning this into a tuple. Same with other descriptions.	chipx86	July 4, 2018, 11:27 p.m.
Are we guaranteed that this is going to be compatible all the time with the args we put in? I'm …	chipx86	July 5, 2018, 9:17 p.m.
Can you make an explicit logger?	chipx86	July 4, 2018, 11:28 p.m.
Same comments as above.	chipx86	July 4, 2018, 11:29 p.m.
Incomplete docstring.	chipx86	July 4, 2018, 11:32 p.m.
Needs 2 blank lines between sections.	chipx86	July 4, 2018, 11:32 p.m.
Can you use a keyword argument for True? I have no idea what this is from reading this code. This …	chipx86	July 4, 2018, 11:32 p.m.
Too many blank lines.	chipx86	July 4, 2018, 11:33 p.m.
These can be combined.	chipx86	July 4, 2018, 11:33 p.m.
These too.	chipx86	July 4, 2018, 11:33 p.m.
Need tests for requesting patch content.	chipx86	July 5, 2018, 9:02 p.m.
Swap these.	chipx86	July 4, 2018, 11:36 p.m.
Blank line between these.	chipx86	July 4, 2018, 11:36 p.m.
E501 line too long (83 > 79 characters)	reviewbot	July 5, 2018, 10:22 p.m.
E127 continuation line over-indented for visual indent	reviewbot	July 5, 2018, 10:22 p.m.
F401 'reviewboard.diffviewer.models.DiffSet' imported but unused	reviewbot	July 25, 2018, 12:43 a.m.
F841 local variable 'diffset' is assigned to but never used	reviewbot	July 25, 2018, 12:43 a.m.
While here, can you change this to "Return" instead of "Returns?"	chipx86	Aug. 28, 2018, 12:06 a.m.
Can you say "without commit history support" ?	chipx86	Aug. 28, 2018, 12:06 a.m.
Can you change this to ReviewRequestDraft.publish?	chipx86	Aug. 28, 2018, 12:07 a.m.
I think this can be reverted. The file isn't otherwise modified, and the blank line is there to keep the …	chipx86	Aug. 28, 2018, 12:07 a.m.
No need for the else:. That will be implied given the return in the if. It'll be nice to flatten …	chipx86	Aug. 28, 2018, 12:08 a.m.
Swap these so they're in alphabetical order.	chipx86	Aug. 28, 2018, 12:08 a.m.
Typo: "sytem" -> "system"	chipx86	Aug. 28, 2018, 12:08 a.m.
You don't need to set this. It will be 'commits', based on name.	chipx86	Aug. 28, 2018, 12:08 a.m.
Should be DictFieldType.	chipx86	Aug. 28, 2018, 12:09 a.m.
The type path must be updated for the new submodule. Same with any others in this change.	chipx86	Aug. 28, 2018, 12:09 a.m.
Should be consistent between here and the previous function with regard to the blank line.	chipx86	Aug. 28, 2018, 12:10 a.m.
Same here.	chipx86	Aug. 28, 2018, 12:10 a.m.
Missing period.	chipx86	Aug. 28, 2018, 12:16 a.m.
You can combine these lines and wrap at a .	chipx86	Aug. 28, 2018, 12:10 a.m.
Missing period.	chipx86	Aug. 28, 2018, 12:11 a.m.
Typo: "Aditional" -> "Additional"	chipx86	Aug. 28, 2018, 12:11 a.m.
Can you pass these as keyword arguments?	chipx86	Aug. 28, 2018, 4:05 a.m.
Typo: "curren" -> "current".	chipx86	Aug. 28, 2018, 12:12 a.m.
These should be in alphabetical order.	chipx86	Aug. 28, 2018, 12:13 a.m.
No newline.	chipx86	Aug. 28, 2018, 12:13 a.m.
Missing a trailing comma.	chipx86	Aug. 28, 2018, 12:13 a.m.
No trailing comma.	chipx86	Aug. 28, 2018, 12:13 a.m.
Typo: "curren" -> "current"	chipx86	Aug. 28, 2018, 12:14 a.m.
Sentence-o: "filter our the" -> "filter the"	chipx86	Aug. 28, 2018, 12:15 a.m.
Missing @webapi_test_template.	chipx86	Aug. 28, 2018, 12:15 a.m.
Missing @webapi_test_template.	chipx86	Aug. 28, 2018, 12:16 a.m.
Alphabetical order.	chipx86	Aug. 28, 2018, 12:16 a.m.
Alphabetical order.	chipx86	Aug. 28, 2018, 12:16 a.m.
This should be the full type path.	chipx86	Aug. 28, 2018, 4:20 a.m.
django, not djblets.	chipx86	Aug. 28, 2018, 3:58 a.m.
This was here before, but can you swap these?	chipx86	Aug. 28, 2018, 3:59 a.m.
Typo: "of" -> "or"	chipx86	Aug. 28, 2018, 3:59 a.m.
Can we leave these to the default serialization so that these date fields are represented the same as all other …	chipx86	Aug. 28, 2018, 4 a.m.
Typo: "specifcally" -> "specifically"	chipx86	Aug. 28, 2018, 4 a.m.
This shouldn't be needed, since you define model_parent_key above and the default implementation just fetches that from the object.	chipx86	Aug. 28, 2018, 4:25 a.m.
Maybe add "in order to post to this resource" at the end of the sentence.	chipx86	Aug. 28, 2018, 4:19 a.m.
Missing trailing comma.	chipx86	Aug. 28, 2018, 4:19 a.m.
Still missing @webapi_test_template.	chipx86	Aug. 28, 2018, 4:20 a.m.
Still missing @webapi_test_template	chipx86	Aug. 28, 2018, 4:20 a.m.
F401 'dateutil.parser.isoparse' imported but unused	reviewbot	Aug. 28, 2018, 4:45 a.m.
F401 'reviewboard.diffviewer.models.DiffCommit' imported but unused	reviewbot	Aug. 28, 2018, 4:45 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/diff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'django.utils.six' imported but unused
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F821 undefined name 'diff_commit'
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F821 undefined name 'diff_commit'
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F821 undefined name 'diff_commit'
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'review_request' is assigned to but never used
```
reviewboard/webapi/tests/test_diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/webapi/tests/test_diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E231 missing whitespace after ','
```
reviewboard/webapi/tests/test_draft_diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'commit' is assigned to but never used
```
reviewboard/webapi/tests/test_draft_diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/webapi/tests/test_draft_diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'draft' is assigned to but never used
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'draft' is assigned to but never used
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F811 redefinition of unused 'test_get_links_dvcs_enabled' from line 268
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'draft' is assigned to but never used
```
reviewboard/webapi/tests/urls.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E302 expected 2 blank lines, found 1
```

Change Summary:

Flake8

Diff:

Revision 2 (+1675 -27)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diff_commit.py
	reviewboard/webapi/resources/draft_diff_commit.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/mimetypes.py
	8 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/diff.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F401 'django.utils.six' imported but unused
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F821 undefined name 'diff_commit'
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F821 undefined name 'diff_commit'
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F821 undefined name 'diff_commit'
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'review_request' is assigned to but never used
```
reviewboard/webapi/tests/test_diff_commit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/webapi/tests/test_diff_commit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E231 missing whitespace after ','
```
reviewboard/webapi/tests/test_draft_diff_commit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'commit' is assigned to but never used
```
reviewboard/webapi/tests/test_draft_diff_commit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/webapi/tests/test_draft_diff_commit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'draft' is assigned to but never used
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'draft' is assigned to but never used
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F811 redefinition of unused 'test_get_links_dvcs_enabled' from line 268
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'draft' is assigned to but never used
```
reviewboard/webapi/tests/urls.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E302 expected 2 blank lines, found 1
```

Commit:

d386c5cd2618b1c382f86d6d6b93f005f3f87d74

ddfec95aff77a82810d82ead65f2320f341cb952

Diff:

Revision 3 (+1677 -27)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diff_commit.py
	reviewboard/webapi/resources/draft_diff_commit.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/mimetypes.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Nest draft diff commit resource under draft diff.
Allow creation of empty diffs.

Commit:

ddfec95aff77a82810d82ead65f2320f341cb952

cca60c9f7b49cf020b701c87922ab46220b48d63

Diff:

Revision 4 (+1808 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diff_commit.py
	reviewboard/webapi/resources/draft_diff.py
	reviewboard/webapi/resources/draft_diff_commit.py
	reviewboard/webapi/resources/review_request_draft.py
	9 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 4)
The issue has been resolved. Show all issues
```
F401 'reviewboard.diffviewer.models.DiffSet' imported but unused
```

Change Summary:

flake8 fixups

Commit:

cca60c9f7b49cf020b701c87922ab46220b48d63

b7d26e370530aa30d3311e6a4f8946a05c9fd1fd

Diff:

Revision 5 (+1807 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diff_commit.py
	reviewboard/webapi/resources/draft_diff.py
	reviewboard/webapi/resources/draft_diff_commit.py
	reviewboard/webapi/resources/review_request_draft.py
	9 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fix some unit test failures. Serialize Diffset.diff_commit_count.

Commit:

b7d26e370530aa30d3311e6a4f8946a05c9fd1fd

80dae83a097b9718f49f3c7bfb1eb0d347fefdda

Diff:

Revision 6 (+1841 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diff_commit.py
	reviewboard/webapi/resources/draft_diff.py
	reviewboard/webapi/resources/draft_diff_commit.py
	reviewboard/webapi/resources/review_request_draft.py
	9 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_diff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```

Change Summary:

Flake8 fixups

Commit:

80dae83a097b9718f49f3c7bfb1eb0d347fefdda

5d84065654c896465bdc821464def60513dbcd15

Diff:

Revision 7 (+1840 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diff_commit.py
	reviewboard/webapi/resources/draft_diff.py
	reviewboard/webapi/resources/draft_diff_commit.py
	reviewboard/webapi/resources/review_request_draft.py
	9 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Remove unit tests that should have been removed when resource nesting changed.

Commit:

5d84065654c896465bdc821464def60513dbcd15

b1b9e794fc66040f62dedc60ec403616aa3e20a0

Diff:

Revision 8 (+1789 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diff_commit.py
	reviewboard/webapi/resources/draft_diff.py
	reviewboard/webapi/resources/draft_diff_commit.py
	reviewboard/webapi/resources/review_request_draft.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Do not serialize diff_commit_count -- it is unnecessary.

Commit:

b1b9e794fc66040f62dedc60ec403616aa3e20a0

5df8b7f7cbb2b0e3de53b010a236637e01936207

Diff:

Revision 9 (+1756 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diff_commit.py
	reviewboard/webapi/resources/draft_diff.py
	reviewboard/webapi/resources/draft_diff_commit.py
	reviewboard/webapi/resources/review_request_draft.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fix unit test failures I thought I fixed earlier

Commit:

5df8b7f7cbb2b0e3de53b010a236637e01936207

469c80901ce5b33252e62b06a88da0ceccc4e8bd

Diff:

Revision 10 (+1749 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diff_commit.py
	reviewboard/webapi/resources/draft_diff.py
	reviewboard/webapi/resources/draft_diff_commit.py
	reviewboard/webapi/resources/review_request_draft.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/forms.py (Diff revision 10)
The issue has been resolved. Show all issues
```
This isn't in the function signature.
```
reviewboard/reviews/forms.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Missing a docstring.
```
reviewboard/reviews/forms.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Too many blank lines.
```

reviewboard/webapi/resources/__init__.py (Diff revision 10)

The issue has been resolved. Show all issues

The and .. or form was used in the past due to lack of the inline conditional support in earlier versions of Python. We should just use the inline form here.

reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
"ID"
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
We shouldn't use literals in summaries.
Same below.
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
You can just use ObjectDoesNotExist here.
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
rsp
```
reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
"solely"
```

reviewboard/webapi/resources/diff_commit.py (Diff revision 10)

The issue has been resolved. Show all issues

Everything is indented 1 space in the body.

reviewboard/webapi/resources/diff_commit.py (Diff revision 10)

The issue has been resolved. Show all issues

Why can't we just tack on ?commit-id=... onto the URL? Are we dealing with dynamic content in the link that could interfere with that?

reviewboard/webapi/resources/diff_commit.py (Diff revision 10)

The issue has been resolved. Show all issues

Let's explicitly compare obj against None, since the former's gotten us into trouble before with valid but falsy-evaluating objects.

reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Can you pass these params as keyword arguments, to help with readability?
```
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Trailing comma is turning this into a tuple.
Same with other descriptions.
```

reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 10)

The issue has been resolved. Show all issues

Are we guaranteed that this is going to be compatible all the time with the args we put in?

I'm tempted to say let's not provide the URL, but rather mention the resource we care about (maybe referencing the link name on the appropriate resource). Callers should not be hard-coding URLs anyway.

reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Can you make an explicit logger?
```
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Same comments as above.
```

reviewboard/webapi/resources/review_request_draft.py (Diff revision 10)

The issue has been resolved. Show all issues

Incomplete docstring.

reviewboard/webapi/tests/mimetypes.py (Diff revision 10)

The issue has been resolved. Show all issues

Needs 2 blank lines between sections.

reviewboard/webapi/tests/test_diff.py (Diff revision 10)

The issue has been resolved. Show all issues

Can you use a keyword argument for True? I have no idea what this is from reading this code.

This applies to all occurrences in the change.

reviewboard/webapi/tests/test_diff.py (Diff revision 10)

The issue has been resolved. Show all issues

Too many blank lines.

reviewboard/webapi/tests/test_diff.py (Diff revision 10)
The issue has been resolved. Show all issues
```
These can be combined.
```
reviewboard/webapi/tests/test_diff.py (Diff revision 10)
The issue has been resolved. Show all issues
```
These too.
```
reviewboard/webapi/tests/test_diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Need tests for requesting patch content.
```
reviewboard/webapi/tests/test_draft_diff.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Swap these.
```
reviewboard/webapi/tests/test_draft_diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Blank line between these.
```

Change Summary:

Addressed issues and rebased.

Commit:

469c80901ce5b33252e62b06a88da0ceccc4e8bd

2b33344865ae2441d22baf9e70be12f2c75da568

Diff:

Revision 11 (+1900 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/draft_diff.py
	reviewboard/webapi/resources/draft_diffcommit.py
	reviewboard/webapi/resources/review_request_draft.py
	8 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/review_request_draft.py (Diff revision 11)
The issue has been resolved. Show all issues
```
E501 line too long (83 > 79 characters)
```
reviewboard/webapi/tests/test_draft_diffcommit.py (Diff revision 11)
The issue has been resolved. Show all issues
```
E127 continuation line over-indented for visual indent
```

Change Summary:

Flake8

Commit:

2b33344865ae2441d22baf9e70be12f2c75da568

3a8fa3d8ae80674c0571d79b29756eb3b5060136

Diff:

Revision 12 (+1901 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/draft_diff.py
	reviewboard/webapi/resources/draft_diffcommit.py
	reviewboard/webapi/resources/review_request_draft.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Update revision after creation of empty diffset.

Commit:

3a8fa3d8ae80674c0571d79b29756eb3b5060136

76cddc14b207db1c81ec5ba6de8061d9d2ac658a

Diff:

Revision 13 (+1903 -68)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/draft_diff.py
	reviewboard/webapi/resources/draft_diffcommit.py
	reviewboard/webapi/resources/review_request_draft.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Add unit tests for the last revision.

Commit:

76cddc14b207db1c81ec5ba6de8061d9d2ac658a

dd3d6d75c3489c09429e005d7eb09837a05a0e05

Diff:

Revision 14 (+1928 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diffcommit.py
	11 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 14)
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 14)
The issue has been resolved. Show all issues
```
F841 local variable 'diffset' is assigned to but never used
```

Change Summary:

Flake8

Commit:

dd3d6d75c3489c09429e005d7eb09837a05a0e05

4b62e014f3b43ae9d3f80c43782c3a02f7b1a52c

Diff:

Revision 15 (+1927 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diffcommit.py
	11 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Allow parent diff in subsequent commits

Commit:

4b62e014f3b43ae9d3f80c43782c3a02f7b1a52c

d1d3f7858022c5cfda0a5ff53f6c778a626cfda5

Diff:

Revision 16 (+1966 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diffcommit.py
	11 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/parser.py (Diff revision 16)
The issue has been resolved. Show all issues
```
While here, can you change this to "Return" instead of "Returns?"
```
reviewboard/reviews/forms.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Can you say "without commit history support" ?
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Can you change this to ReviewRequestDraft.publish?
```

reviewboard/testing/testcase.py (Diff revision 16)

The issue has been resolved. Show all issues

I think this can be reverted. The file isn't otherwise modified, and the blank line is there to keep the comment tied specifically to the setting of the ID.

reviewboard/webapi/resources/diff.py (Diff revision 16)

The issue has been resolved. Show all issues

No need for the else:. That will be implied given the return in the if. It'll be nice to flatten this a bit.

reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Swap these so they're in alphabetical order.
```
reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Typo: "sytem" -> "system"
```
reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
You don't need to set this. It will be 'commits', based on name.
```
reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Should be DictFieldType.
```
reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
The type path must be updated for the new submodule.
Same with any others in this change.
```

reviewboard/webapi/resources/diffcommit.py (Diff revision 16)

The issue has been resolved. Show all issues

Should be consistent between here and the previous function with regard to the blank line.

reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Missing period.
```
reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
You can combine these lines and wrap at a .
```
reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Missing period.
```
reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Typo: "Aditional" -> "Additional"
```

reviewboard/webapi/resources/diffcommit.py (Diff revision 16)

The issue has been dropped. Show all issues

Can you pass these as keyword arguments?

reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Typo: "curren" -> "current".
```
reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
These should be in alphabetical order.
```
reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
No newline.
```
reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```
reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
No trailing comma.
```
reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Typo: "curren" -> "current"
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Sentence-o: "filter our the" -> "filter the"
```
reviewboard/webapi/tests/test_diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Missing @webapi_test_template.
```
reviewboard/webapi/tests/test_diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Missing @webapi_test_template.
```
reviewboard/webapi/tests/test_draft_diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Alphabetical order.
```
reviewboard/webapi/tests/test_draft_diffcommit.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Alphabetical order.
```

Change Summary:

Addressed Christian's feedback.

Commit:

d1d3f7858022c5cfda0a5ff53f6c778a626cfda5

c0b700800a0acd74cf7f54beea88b8035e08b226

Diff:

Revision 17 (+1966 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/draft_diff.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/forms.py (Diff revision 17)
The issue has been resolved. Show all issues
```
This should be the full type path.
```
reviewboard/reviews/forms.py (Diff revision 17)
The issue has been resolved. Show all issues
```
django, not djblets.
```
reviewboard/webapi/resources/__init__.py (Diff revision 17)
The issue has been resolved. Show all issues
```
This was here before, but can you swap these?
```
reviewboard/webapi/resources/diffcommit.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Typo: "of" -> "or"
```

reviewboard/webapi/resources/diffcommit.py (Diff revision 17)

The issue has been resolved. Show all issues

Can we leave these to the default serialization so that these date fields are represented the same as all other date fields in the API?

reviewboard/webapi/resources/diffcommit.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Typo: "specifcally" -> "specifically"
```

reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 17)

The issue has been resolved. Show all issues

This shouldn't be needed, since you define model_parent_key above and the default implementation just fetches that from the object.

reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Maybe add "in order to post to this resource" at the end of the sentence.
```
reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Missing trailing comma.
```
reviewboard/webapi/tests/test_diffcommit.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Still missing @webapi_test_template.
```
reviewboard/webapi/tests/test_diffcommit.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Still missing @webapi_test_template
```

Change Summary:

address feedback

Commit:

c0b700800a0acd74cf7f54beea88b8035e08b226

d79ea8ea8a89b776461fb1e85c7ad62246739580

Diff:

Revision 18 (+1918 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/draft_diff.py
	10 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_diffcommit.py (Diff revision 18)
The issue has been resolved. Show all issues
```
F401 'dateutil.parser.isoparse' imported but unused
```
reviewboard/webapi/tests/test_diffcommit.py (Diff revision 18)
The issue has been resolved. Show all issues
```
F401 'reviewboard.diffviewer.models.DiffCommit' imported but unused
```

Ship it!

```
I'm happy once Review Bot is.
```

Commit:

d79ea8ea8a89b776461fb1e85c7ad62246739580

3118383a3714171a43636e2f2f5f6eca52f9ef47

Diff:

Revision 19 (+1916 -69)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/diff.py
	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/draft_diff.py
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status:: Completed
Change Summary:: Pushed to release-4.0.x (f03566e)