Add initial web API for commit histories.
Review Request #6816 — Created Jan. 20, 2015 and submitted
Add the
DiffCommitResource
andDraftDiffCommitResource
resources.
TheDiffCommitResource
is a resource for retrieving the information
about theDiffCommits
under a particularDiffSet
. The
DraftDiffCommitResource
inherits from theDiffCommitResource
and
modifies it to allow for the creation ofDiffCommits
and associated
FileDiffs
. TheDraftDiffCommitResource
handles the uploading of
FileDiffs
forDiffCommits
instead of theDiffSetResource
.The
DiffSetResource
POST method now takes an extra optional field,
create_empty
, that determines if the DiffSet that is created is
created without uploading a.diff file
. This is used in the creation
of review requests with commit histories.The
FileDiffResource
andDraftFileDiffResource
classes now filter
their results based on the commit query parameter. If no commit query
parameter is given, only thoseFileDiffs
unattached to a
DiffCommit
will be return. This filtering is only applied to the
list API, not the item API.Add a new form, the
UploadCommitForm
, for handling the processing of
data for creating newDiffCommit
instances.Refactor behaviour for uploading
FileDiffs
from theDiffSetManager
to a new class, theDiffProcessorMixin
. This mix-in handles the
initial processing of.diff
files, their comparison (for header vs.
source files), generating file lists, and creating actualFileDiffs
from upload (it calls the child class's implementation of
create_from_data
).The
DiffSetManager
now has acreate_empty
method for creating
DiffSets
withoutFileDiffs
for usage withDiffCommits
.Add the
DiffCommitManager
model. It inherits from the
DiffProcessorMixin
.Add a WebAPI capability to show that the server supports review
requests with commit histories. This allows new versions ofrbt
that
support commit histories to create review requests with history by
default if the tool supports it.Add new unit tests for the web API. The tests include:
-test_filediff
, which testsFileDiff
filtering of aDiffCommit
-test_draft_diff_commit
, which does the base API tests for the
DraftDiffCommitResource
-test_diff_commit
, which does the base API tests for the
DiffCommitResource
Unit tests pass.
Did the following manual testing:
- Created a review request via
rbt post
. It was automatically
created with history. (There was one commit under the draft). - Able to read the commit list and single commit via the API.
- Able to filter the
FileDiff
list resource. - The
FileDiff
item resource was not filtered. - Able to publish, discard, and delete the review request.
- Able to use all the public API endoint functionality of the
published RR anonymously
Description | From | Last Updated |
---|---|---|
No blank line here. |
chipx86 | |
Nit - we like to put these in alphabetical order. from reviewboard.diffviewer.forms... should go before from reviewboard.diffviewer.models... |
mike_conley | |
We also like trailing commas! :D |
mike_conley | |
Is this necessary? Like, if we provide no file for the path parameter, doesn't that essentially mean "Create an empty … |
mike_conley | |
Maybe specifically say "commit history," so that people looking at the docs don't think this is required for standard diff … |
chipx86 | |
You should be able to put this in the argument list for the function. |
chipx86 | |
What are we using this for in the form? Any way we can allow DiffSet.save to deal with this? No … |
chipx86 | |
Can you flesh this out more? This directly turns into documentation on the website for the API, so we should … |
chipx86 | |
We should probably say that this is a list of IDs. |
chipx86 | |
Instead of describing this in the docs, you can do: 'commit_type': { 'type': ('M', 'C'), ... } It will then … |
chipx86 | |
Missing period. |
chipx86 | |
If all we need are the commit_id values, we can reduce the query and deserialization overhead by doing: return obj.merge_parent_ids.values_list('commit_id', … |
chipx86 | |
Missing period. |
chipx86 | |
As above, docstrings for this and the other handler methods will all be turned into public documentation on the website. … |
chipx86 | |
I wouldn't expect this to be needed. It should be handled by the parent resource. |
chipx86 | |
Same as above. |
chipx86 | |
This can be one line. |
chipx86 | |
Maybe ValueError? I would also log this, so that it's easier for admins to see the issue. Use exc_info=1 with … |
chipx86 | |
No blank line. |
chipx86 | |
""" on the next line. |
chipx86 | |
If accessing this with a query parameter, you can end up with something like <url>/?existing-param=123?commit_id=.... We need to check if … |
chipx86 | |
This will be used in the public documentation on the site, so the internal-only extra_fields doesn't mean much. I'd say … |
chipx86 | |
Could simplify just a tad with: if '?' in url: url += '&' else: url += '?' return '%scommit-id=%s' % … |
chipx86 | |
These should be listed in alphabetical order. |
chipx86 | |
These are probably copy/pasted from the other file, but these should also be in alphabetical order. |
chipx86 | |
This one too. |
chipx86 | |
Same comments on docs will apply here and elsewhere. |
chipx86 | |
This is one of those optimization tricks you learn over time, but this would be easier on the database if … |
chipx86 | |
As this is the DraftDiffCommitResource, Do we not need to ensure that the user associated with the request can view … |
mike_conley | |
This should go above the HTTP handlers. |
chipx86 | |
As above, you can use a tuple of allowed values. And also, we should probably use "merge" or "change". |
chipx86 | |
Nit - please add a trailing comma |
mike_conley | |
Nit - please add a trailing comma |
mike_conley | |
list? |
chipx86 | |
I might be missing it, but what's expecting this to be {}? The usage below checking if it's valid only … |
chipx86 | |
No blank line between try/excepts. |
chipx86 | |
Same here. |
chipx86 | |
This is an internal detail, but not one that's useful to consumers of the API. We should avoid talking about … |
chipx86 | |
'commit-id' |
chipx86 | |
This looks the same as the addition in DraftFileDiffResource. Any way to DRY it up? |
mike_conley | |
This might be better suited to diffs above. |
chipx86 | |
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot | |
You probably didn't mean to leave this here. |
mike_conley | |
Blank line here. |
chipx86 | |
Missing trailing slash. |
chipx86 | |
'DiffCommit' imported but unused |
reviewbot | |
'resources' imported but unused |
reviewbot | |
Col: 5 E265 block comment should start with '# ' |
reviewbot | |
Left over from something? |
chipx86 | |
No blank line needed here |
chipx86 | |
Col: 1 E303 too many blank lines (3) |
reviewbot | |
'DiffCommit' imported but unused |
reviewbot | |
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot | |
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot | |
'DiffSet' imported but unused |
reviewbot | |
This will be directly rendered as ReST into the docs, so we should place double backticks around with_history and path, … |
chipx86 | |
No blank line here. |
chipx86 | |
Maybe "Each diff commit resource contains ..." |
chipx86 | |
"to reproduce" |
chipx86 | |
This might be mistaken for a hash. Maybe "The numeric ID of the commit resource." |
chipx86 | |
"The e-mail address of ..." Other docs use "e-mail address," so we should be consistent there. |
chipx86 | |
I wonder if we want to say "commit message" instead? |
chipx86 | |
"DiffSets" are internal. Let's just say "on a particular commit." |
chipx86 | |
Technically, it also has things like likes, extra_data, etc. Maybe we don't want to say "currently only," but instead something … |
chipx86 | |
Same note about DiffSets. |
chipx86 | |
"DiffCommit" is also internal, so let's say "Updates information on the commit." |
chipx86 | |
Too many blank lines. |
chipx86 | |
"resource" |
chipx86 | |
Same comment regarding internal names. I don't think, from the consumer's perspective, that we even need this last sentence, since … |
chipx86 | |
I'd remove "... in a DiffSet." |
chipx86 | |
Same comment here as in the other resource. |
chipx86 | |
"... pertaining to the diff revision." ? |
chipx86 | |
"e-mail address" |
chipx86 | |
"e-mail address" |
chipx86 | |
"DiffCommit" is an internal name. |
chipx86 | |
"DiffSet" is an internal name. Here and elsewhere in this docstring. |
chipx86 | |
No blank line. |
chipx86 | |
This line should no longer be needed. |
chipx86 | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot | |
Col: 5 E303 too many blank lines (2) |
reviewbot | |
Col: 5 E303 too many blank lines (2) |
reviewbot | |
'os' imported but unused |
reviewbot | |
'scmtools' imported but unused |
reviewbot | |
local variable 'diff_filename' is assigned to but never used |
reviewbot | |
'os' imported but unused |
reviewbot | |
'scmtools' imported but unused |
reviewbot | |
This could just be if has_path == with_history |
david | |
This should end in a period. |
david | |
Missing period. |
chipx86 | |
"commits" |
chipx86 | |
"so that it's created empty" |
chipx86 | |
Maybe verify it is indeed one of the valid values? |
chipx86 | |
Missing a trailing slash. |
chipx86 |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py
-
reviewboard/webapi/tests/test_diff_commit.py (Diff revision 1) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/webapi/tests/test_filediff.py (Diff revision 1) Col: 5 E265 block comment should start with '# '
-
-
reviewboard/webapi/resources/diff.py (Diff revision 1) Nit - we like to put these in alphabetical order.
from reviewboard.diffviewer.forms...
should go before
from reviewboard.diffviewer.models...
-
-
reviewboard/webapi/resources/diff.py (Diff revision 1) Is this necessary? Like, if we provide no file for the path parameter, doesn't that essentially mean "Create an empty DiffResource"?
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 1) As this is the DraftDiffCommitResource, Do we not need to ensure that the user associated with the request can view this information (ie, be at least staff level, or be the submitter)?
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 1) Nit - please add a trailing comma
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 1) Nit - please add a trailing comma
-
reviewboard/webapi/resources/filediff.py (Diff revision 1) This looks the same as the addition in DraftFileDiffResource. Any way to DRY it up?
-
reviewboard/webapi/tests/test_diff_commit.py (Diff revision 1) You probably didn't mean to leave this here.
-
-
-
reviewboard/webapi/resources/diff.py (Diff revision 1) What are we using this for in the form? Any way we can allow
DiffSet.save
to deal with this? No big deal if not (there's unlikely to be a race condition in this particular case), but worth asking.Also, since this is very close to the logic in
DiffSet.save
, maybe we can just extract that to a function onDiffSetManager
and use that in both places? -
reviewboard/webapi/resources/diff_commit.py (Diff revision 1) Can you flesh this out more? This directly turns into documentation on the website for the API, so we should be as clear and explicit about the purposes of the resource as we can.
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 1) We should probably say that this is a list of IDs.
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 1) Instead of describing this in the docs, you can do:
'commit_type': { 'type': ('M', 'C'), ... }
It will then automatically be validated when the user posts, without you having to do anything.
I'd prefer we use "merge" or "change" though, instead of "M" or "C".
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 1) If all we need are the commit_id values, we can reduce the query and deserialization overhead by doing:
return obj.merge_parent_ids.values_list('commit_id', flat=True)
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 1) As above, docstrings for this and the other handler methods will all be turned into public documentation on the website. They should contain more detailed information. You can see examples in other resources.
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 1) I wouldn't expect this to be needed. It should be handled by the parent resource.
-
-
-
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 1) If accessing this with a query parameter, you can end up with something like
<url>/?existing-param=123?commit_id=...
. We need to check if?
is already in there, and then choose between?
and&
.Also, this should be
commit-id=
. We don't use_
(except in a couple legacy cases) in query strings. -
reviewboard/webapi/resources/draft_diff.py (Diff revision 1) These should be listed in alphabetical order.
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 1) These are probably copy/pasted from the other file, but these should also be in alphabetical order.
-
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 1) Same comments on docs will apply here and elsewhere.
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 1) This is one of those optimization tricks you learn over time, but this would be easier on the database if you do:
draft = ReviewRequestDraft.objects.get(diffset__commits=commit) return draft.is_accessible_by(request.user)
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 1) This should go above the HTTP handlers.
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 1) As above, you can use a tuple of allowed values. And also, we should probably use "merge" or "change".
-
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 1) I might be missing it, but what's expecting this to be
{}
? The usage below checking if it's valid only checks for truthiness. -
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 1) No blank line between try/excepts.
-
-
-
-
-
-
-
Change Summary:
Address issues.
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/draft_diff.py reviewboard/testing/testcase.py reviewboard/webapi/server_info.py reviewboard/diffviewer/tests.py reviewboard/webapi/resources/validate_diff.py reviewboard/diffviewer/evolutions/__init__.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/draft_filediff.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/diffviewer/managers.py reviewboard/diffviewer/admin.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/diffviewer/models.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/tests/test_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/draft_diff.py reviewboard/testing/testcase.py reviewboard/webapi/server_info.py reviewboard/diffviewer/tests.py reviewboard/webapi/resources/validate_diff.py reviewboard/diffviewer/evolutions/__init__.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/draft_filediff.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/diffviewer/managers.py reviewboard/diffviewer/admin.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/diffviewer/models.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/tests/test_filediff.py
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 2) Col: 1 E303 too many blank lines (3)
-
Change Summary:
Update wrt change to r/6815. All that needs updating is the docstrings.
Diff: |
Revision 3 (+1959 -205)
|
---|
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/draft_diff.py reviewboard/testing/testcase.py reviewboard/webapi/server_info.py reviewboard/diffviewer/tests.py reviewboard/webapi/resources/validate_diff.py reviewboard/diffviewer/evolutions/__init__.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/draft_filediff.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/diffviewer/managers.py reviewboard/diffviewer/admin.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/diffviewer/models.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/tests/test_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/draft_diff.py reviewboard/testing/testcase.py reviewboard/webapi/server_info.py reviewboard/diffviewer/tests.py reviewboard/webapi/resources/validate_diff.py reviewboard/diffviewer/evolutions/__init__.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/draft_filediff.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/diffviewer/managers.py reviewboard/diffviewer/admin.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/diffviewer/models.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/tests/test_filediff.py
-
reviewboard/webapi/tests/test_diff_commit.py (Diff revision 3) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/webapi/tests/test_diff_commit.py (Diff revision 3) Col: 1 E302 expected 2 blank lines, found 1
Change Summary:
Update docstrings.
Ensure parent diffset is empty before commit upload.
Add test for new behaviour.
Diff: |
Revision 4 (+1205 -48)
|
---|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py
-
Change Summary:
PEP8
Diff: |
Revision 5 (+1204 -48)
|
---|
-
Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py
-
-
reviewboard/webapi/resources/diff.py (Diff revisions 1 - 5) Maybe specifically say "commit history," so that people looking at the docs don't think this is required for standard diff revision stuff.
-
reviewboard/webapi/resources/diff.py (Diff revisions 1 - 5) You should be able to put this in the argument list for the function.
-
-
-
reviewboard/webapi/resources/diff_commit.py (Diff revisions 1 - 5) Maybe
ValueError
?I would also log this, so that it's easier for admins to see the issue. Use
exc_info=1
with that as well. -
-
reviewboard/webapi/resources/diff_commit.py (Diff revisions 1 - 5) This will be used in the public documentation on the site, so the internal-only
extra_fields
doesn't mean much.I'd say something like:
"""Updates information on the DiffCommit. Custom data can be attached to the DiffCommit by prefixing fields with "extra_data." and a field name. <some more notes about namespacing maybe...>
We should probably have general docs on extra_data stuff, which we could then link to, but that doesn't have to be now of course.
-
reviewboard/webapi/resources/diff_commit.py (Diff revisions 1 - 5) Could simplify just a tad with:
if '?' in url: url += '&' else: url += '?' return '%scommit-id=%s' % (url, commit)
(Python performance tip: String formatters are faster than string concatenation.)
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revisions 1 - 5) This is an internal detail, but not one that's useful to consumers of the API. We should avoid talking about any internal objects or functions. We can, however, link to other API docs using:
:ref:`webapi2.0-<name>`
Change Summary:
Update wrt most of Christian's issues.
Diff: |
Revision 6 (+1225 -50)
|
---|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py
-
The code's looking to be in good shape. A couple small things I noticed, but nothing major.
My primary focus was on the docs. Biggest thing there is that a lot of the docs are documenting from the point of view of someone working with the codebase, but API docs should speak from a different vantage point. They shouldn't reference internal objects or resource names, but rather should speak more generally about resources and concepts. Things like how objects are attached internally aren't as useful there. So, a few comments regarding those, but nothing too big I don't think. :)
-
reviewboard/webapi/resources/diff.py (Diff revisions 5 - 6) This will be directly rendered as ReST into the docs, so we should place double backticks around
with_history
andpath
, here and below. -
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 6) Maybe "Each diff commit resource contains ..."
-
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 6) This might be mistaken for a hash. Maybe "The numeric ID of the commit resource."
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 6) "The e-mail address of ..."
Other docs use "e-mail address," so we should be consistent there.
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 6) I wonder if we want to say "commit message" instead?
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 6) "DiffSets" are internal. Let's just say "on a particular commit."
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 6) Technically, it also has things like likes, extra_data, etc. Maybe we don't want to say "currently only," but instead something like:
This provides the metadata associated with the commit, such as the commit message, author, revisions, and other information available on the commit.
-
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 6) "DiffCommit" is also internal, so let's say "Updates information on the commit."
-
-
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 6) Same comment regarding internal names. I don't think, from the consumer's perspective, that we even need this last sentence, since it's about what happens internally.
-
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 6) Same comment here as in the other resource.
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 6) "... pertaining to the diff revision." ?
-
-
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 6) "DiffCommit" is an internal name.
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 6) "DiffSet" is an internal name. Here and elsewhere in this docstring.
-
-
Change Summary:
Fixups.
Diff: |
Revision 7 (+1227 -50)
|
---|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py
-
reviewboard/webapi/resources/diff_commit.py (Diff revision 7) Col: 80 E501 line too long (80 > 79 characters)
-
reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 7) Col: 80 E501 line too long (80 > 79 characters)
Change Summary:
Updated à la PEP 8
Diff: |
Revision 8 (+1227 -50)
|
---|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py
Change Summary:
Fix error message.
Diff: |
Revision 9 (+1227 -50)
|
---|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py
Change Summary:
Decouple upload forms into a mixin because they require different params for initialization.
Fix the case where updating the review request by uploading new diffs breaks everything.
Diff: |
Revision 10 (+1279 -66) |
---|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py
-
-
-
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/validate_diff.py reviewboard/webapi/resources/draft_filediff.py
Change Summary:
Moved validate endpoint fix into previous RR (because it was breaking unit tests)
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+1276 -64) |
Change Summary:
Formatting of testing done
Testing Done: |
|
---|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/draft_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/draft_filediff.py
Change Summary:
Change tests to use hardcoded test data.
Commit: |
|
||
---|---|---|---|
Diff: |
Revision 13 (+1229 -64) |
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/draft_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/draft_filediff.py
-
-
-
reviewboard/webapi/tests/test_draft_diff_commit.py (Diff revision 13) local variable 'diff_filename' is assigned to but never used
-
-
Change Summary:
PEP8
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+1218 -64) |
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/draft_filediff.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/draft_filediff.py
-
This is looking pretty solid!
-
reviewboard/webapi/resources/diff.py (Diff revision 14) This could just be
if has_path == with_history
-
Change Summary:
Fix David's issues.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+1218 -64) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/draft_filediff.py Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/draft_filediff.py
-
Only a few very minor things left (typos, basically). All the rest of this looks very good! :D
-
-
-
-
reviewboard/webapi/tests/test_diff_commit.py (Diff revision 15) Maybe verify it is indeed one of the valid values?
-
-
Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/draft_filediff.py
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/draft_diff.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/draft_diff_commit.py reviewboard/webapi/server_info.py reviewboard/webapi/resources/filediff.py reviewboard/webapi/tests/test_draft_diff_commit.py reviewboard/webapi/tests/test_diff_commit.py reviewboard/reviews/forms.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/test_filediff.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/diff.py reviewboard/webapi/resources/diff_commit.py reviewboard/diffviewer/forms.py reviewboard/webapi/resources/draft_filediff.py