Add initial web API for commit histories.

Review Request #6816 — Created Jan. 20, 2015 and submitted

Information

Review Board
dvcs
617622a...

Reviewers

Add the DiffCommitResource and DraftDiffCommitResource resources.
The DiffCommitResource is a resource for retrieving the information
about the DiffCommits under a particular DiffSet. The
DraftDiffCommitResource inherits from the DiffCommitResource and
modifies it to allow for the creation of DiffCommits and associated
FileDiffs. The DraftDiffCommitResource handles the uploading of
FileDiffs for DiffCommits instead of the DiffSetResource.

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 and DraftFileDiffResource classes now filter
their results based on the commit query parameter. If no commit query
parameter is given, only those FileDiffs 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 new DiffCommit instances.

Refactor behaviour for uploading FileDiffs from the DiffSetManager
to a new class, the DiffProcessorMixin. This mix-in handles the
initial processing of .diff files, their comparison (for header vs.
source files), generating file lists, and creating actual FileDiffs
from upload (it calls the child class's implementation of
create_from_data).

The DiffSetManager now has a create_empty method for creating
DiffSets without FileDiffs for usage with DiffCommits.

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 of rbt 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 tests FileDiff filtering of a DiffCommit
- 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.

chipx86chipx86

Nit - we like to put these in alphabetical order. from reviewboard.diffviewer.forms... should go before from reviewboard.diffviewer.models...

mike_conleymike_conley

We also like trailing commas! :D

mike_conleymike_conley

Is this necessary? Like, if we provide no file for the path parameter, doesn't that essentially mean "Create an empty …

mike_conleymike_conley

Maybe specifically say "commit history," so that people looking at the docs don't think this is required for standard diff …

chipx86chipx86

You should be able to put this in the argument list for the function.

chipx86chipx86

What are we using this for in the form? Any way we can allow DiffSet.save to deal with this? No …

chipx86chipx86

Can you flesh this out more? This directly turns into documentation on the website for the API, so we should …

chipx86chipx86

We should probably say that this is a list of IDs.

chipx86chipx86

Instead of describing this in the docs, you can do: 'commit_type': { 'type': ('M', 'C'), ... } It will then …

chipx86chipx86

Missing period.

chipx86chipx86

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', …

chipx86chipx86

Missing period.

chipx86chipx86

As above, docstrings for this and the other handler methods will all be turned into public documentation on the website. …

chipx86chipx86

I wouldn't expect this to be needed. It should be handled by the parent resource.

chipx86chipx86

Same as above.

chipx86chipx86

This can be one line.

chipx86chipx86

Maybe ValueError? I would also log this, so that it's easier for admins to see the issue. Use exc_info=1 with …

chipx86chipx86

No blank line.

chipx86chipx86

""" on the next line.

chipx86chipx86

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 …

chipx86chipx86

This will be used in the public documentation on the site, so the internal-only extra_fields doesn't mean much. I'd say …

chipx86chipx86

Could simplify just a tad with: if '?' in url: url += '&' else: url += '?' return '%scommit-id=%s' % …

chipx86chipx86

These should be listed in alphabetical order.

chipx86chipx86

These are probably copy/pasted from the other file, but these should also be in alphabetical order.

chipx86chipx86

This one too.

chipx86chipx86

Same comments on docs will apply here and elsewhere.

chipx86chipx86

This is one of those optimization tricks you learn over time, but this would be easier on the database if …

chipx86chipx86

As this is the DraftDiffCommitResource, Do we not need to ensure that the user associated with the request can view …

mike_conleymike_conley

This should go above the HTTP handlers.

chipx86chipx86

As above, you can use a tuple of allowed values. And also, we should probably use "merge" or "change".

chipx86chipx86

Nit - please add a trailing comma

mike_conleymike_conley

Nit - please add a trailing comma

mike_conleymike_conley

list?

chipx86chipx86

I might be missing it, but what's expecting this to be {}? The usage below checking if it's valid only …

chipx86chipx86

No blank line between try/excepts.

chipx86chipx86

Same here.

chipx86chipx86

This is an internal detail, but not one that's useful to consumers of the API. We should avoid talking about …

chipx86chipx86

'commit-id'

chipx86chipx86

This looks the same as the addition in DraftFileDiffResource. Any way to DRY it up?

mike_conleymike_conley

This might be better suited to diffs above.

chipx86chipx86

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

You probably didn't mean to leave this here.

mike_conleymike_conley

Blank line here.

chipx86chipx86

Missing trailing slash.

chipx86chipx86

'DiffCommit' imported but unused

reviewbotreviewbot

'resources' imported but unused

reviewbotreviewbot

Col: 5 E265 block comment should start with '# '

reviewbotreviewbot

Left over from something?

chipx86chipx86

No blank line needed here

chipx86chipx86

Col: 1 E303 too many blank lines (3)

reviewbotreviewbot

'DiffCommit' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

'DiffSet' imported but unused

reviewbotreviewbot

This will be directly rendered as ReST into the docs, so we should place double backticks around with_history and path, …

chipx86chipx86

No blank line here.

chipx86chipx86

Maybe "Each diff commit resource contains ..."

chipx86chipx86

"to reproduce"

chipx86chipx86

This might be mistaken for a hash. Maybe "The numeric ID of the commit resource."

chipx86chipx86

"The e-mail address of ..." Other docs use "e-mail address," so we should be consistent there.

chipx86chipx86

I wonder if we want to say "commit message" instead?

chipx86chipx86

"DiffSets" are internal. Let's just say "on a particular commit."

chipx86chipx86

Technically, it also has things like likes, extra_data, etc. Maybe we don't want to say "currently only," but instead something …

chipx86chipx86

Same note about DiffSets.

chipx86chipx86

"DiffCommit" is also internal, so let's say "Updates information on the commit."

chipx86chipx86

Too many blank lines.

chipx86chipx86

"resource"

chipx86chipx86

Same comment regarding internal names. I don't think, from the consumer's perspective, that we even need this last sentence, since …

chipx86chipx86

I'd remove "... in a DiffSet."

chipx86chipx86

Same comment here as in the other resource.

chipx86chipx86

"... pertaining to the diff revision." ?

chipx86chipx86

"e-mail address"

chipx86chipx86

"e-mail address"

chipx86chipx86

"DiffCommit" is an internal name.

chipx86chipx86

"DiffSet" is an internal name. Here and elsewhere in this docstring.

chipx86chipx86

No blank line.

chipx86chipx86

This line should no longer be needed.

chipx86chipx86

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

'os' imported but unused

reviewbotreviewbot

'scmtools' imported but unused

reviewbotreviewbot

local variable 'diff_filename' is assigned to but never used

reviewbotreviewbot

'os' imported but unused

reviewbotreviewbot

'scmtools' imported but unused

reviewbotreviewbot

This could just be if has_path == with_history

daviddavid

This should end in a period.

daviddavid

Missing period.

chipx86chipx86

"commits"

chipx86chipx86

"so that it's created empty"

chipx86chipx86

Maybe verify it is indeed one of the valid values?

chipx86chipx86

Missing a trailing slash.

chipx86chipx86
reviewbot
  1. 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
    
    
  2. Show all issues
     'DiffCommit' imported but unused
    
  3. Show all issues
     'resources' imported but unused
    
  4. 
      
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. Show all issues
    Col: 5
     E265 block comment should start with '# '
    
  4. 
      
brennie
brennie
mike_conley
  1. 
      
  2. reviewboard/webapi/resources/diff.py (Diff revision 1)
     
     
    Show all issues

    Nit - we like to put these in alphabetical order.

    from reviewboard.diffviewer.forms...
    

    should go before

    from reviewboard.diffviewer.models...
    
  3. reviewboard/webapi/resources/diff.py (Diff revision 1)
     
     
    Show all issues

    We also like trailing commas! :D

  4. reviewboard/webapi/resources/diff.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    Is this necessary? Like, if we provide no file for the path parameter, doesn't that essentially mean "Create an empty DiffResource"?

    1. Oops. That wasn't supposed to be empty.

      The idea with the create_empty parameter is that we want the client to KNOW it is creating a review request involving commit history. It could be possible for incompatible tools to go screw up uploading a file diff and create empty diffsets accidentally.

    2. Hmm, I can see both sides here.

      How about a third object: Some sort of "type" field that specifies if it's for a single diff or a commit set, defaulting to a single diff? We could then do the appropriate validation. Also gives us room for future expansion, if needed.

    3. I think that in the future, we can refactor this into a new type field if necessary. If we add a type field, then one of the types is old (or similar) and the other type is new (or similar). This must default to the old type, but we can't require its existence if the version of RBTools is old. So it basically is identical to the create_empty field.

  5. Show all issues

    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)?

    1. Thats what the has_access_permissions and has_list_access_permissions functions are for.

  6. Show all issues

    Nit - please add a trailing comma

  7. Show all issues

    Nit - please add a trailing comma

  8. reviewboard/webapi/resources/filediff.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This looks the same as the addition in DraftFileDiffResource. Any way to DRY it up?

    1. Refactored the common code into a new method :)

  9. Show all issues

    You probably didn't mean to leave this here.

  10. 
      
chipx86
  1. 
      
  2. reviewboard/testing/testcase.py (Diff revision 1)
     
     
     
     
    Show all issues

    No blank line here.

  3. reviewboard/webapi/resources/diff.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    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 on DiffSetManager and use that in both places?

  4. Show all issues

    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.

  5. reviewboard/webapi/resources/diff_commit.py (Diff revision 1)
     
     
     
    Show all issues

    We should probably say that this is a list of IDs.

  6. reviewboard/webapi/resources/diff_commit.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    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".

    1. The posting only happens on the DraftDiffCommit.create method so should I put that on there instead of here?

    2. Both places. The one for create will be shown on the list of possible fields for a POST in the docs, and the one in fields will be shown for the resource payload fields docs.

  7. Show all issues

    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)
    
  8. Show all issues

    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.

  9. reviewboard/webapi/resources/diff_commit.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    I wouldn't expect this to be needed. It should be handled by the parent resource.

  10. reviewboard/webapi/resources/diff_commit.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    Same as above.

  11. reviewboard/webapi/resources/diff_commit.py (Diff revision 1)
     
     
     
     
    Show all issues

    This can be one line.

  12. reviewboard/webapi/resources/diff_commit.py (Diff revision 1)
     
     
     
     
    Show all issues

    No blank line.

  13. Show all issues

    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.

  14. reviewboard/webapi/resources/draft_diff.py (Diff revision 1)
     
     
     
    Show all issues

    These should be listed in alphabetical order.

  15. Show all issues

    These are probably copy/pasted from the other file, but these should also be in alphabetical order.

  16. Show all issues

    This one too.

  17. Show all issues

    Same comments on docs will apply here and elsewhere.

  18. Show all issues

    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)
    
    1. This method won't actually work because diff_commits is a related field generated by the ORM.

    2. Never mind me :)

  19. Show all issues

    This should go above the HTTP handlers.

  20. Show all issues

    As above, you can use a tuple of allowed values. And also, we should probably use "merge" or "change".

  21. Show all issues

    list?

    1. When you post to the resource, we are sending a comma-seperated string. Should this be list or text type?

    2. Sorry, I had this wrong. For posting, it'll need to be a comma-separated string, not a list. Looking at webapi_request_fields, list actually implies a set of specific choices, same as the tuple I mentioned above.

      (We may want to extend Djblets's support at some point to have some type indicating comma-separated lists...)

  22. Show all issues

    I might be missing it, but what's expecting this to be {}? The usage below checking if it's valid only checks for truthiness.

  23. Show all issues

    No blank line between try/excepts.

  24. Show all issues

    Same here.

  25. Show all issues

    'commit-id'

  26. reviewboard/webapi/server_info.py (Diff revision 1)
     
     
    Show all issues

    This might be better suited to diffs above.

  27. Show all issues

    Blank line here.

  28. Show all issues

    Missing trailing slash.

  29. reviewboard/webapi/tests/test_filediff.py (Diff revision 1)
     
     
     
    Show all issues

    Left over from something?

  30. reviewboard/webapi/tests/test_filediff.py (Diff revision 1)
     
     
     
     
    Show all issues

    No blank line needed here

  31. 
      
brennie
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 1
     E303 too many blank lines (3)
    
  3. Show all issues
     'DiffCommit' imported but unused
    
  4. 
      
brennie
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. 
      
brennie
reviewbot
  1. 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
    
    
  2. Show all issues
     'DiffSet' imported but unused
    
  3. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/resources/diff.py (Diff revisions 1 - 5)
     
     
     
    Show all issues

    Maybe specifically say "commit history," so that people looking at the docs don't think this is required for standard diff revision stuff.

  3. reviewboard/webapi/resources/diff.py (Diff revisions 1 - 5)
     
     
    Show all issues

    You should be able to put this in the argument list for the function.

  4. reviewboard/webapi/resources/diff_commit.py (Diff revisions 1 - 5)
     
     
    Show all issues

    Missing period.

  5. reviewboard/webapi/resources/diff_commit.py (Diff revisions 1 - 5)
     
     
    Show all issues

    Missing period.

  6. reviewboard/webapi/resources/diff_commit.py (Diff revisions 1 - 5)
     
     
    Show all issues

    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.

    1. How can we log with exc_info if we are the ones raising the exception?

    2. Good point. Just log I guess :)

  7. reviewboard/webapi/resources/diff_commit.py (Diff revisions 1 - 5)
     
     
    Show all issues

    """ on the next line.

  8. reviewboard/webapi/resources/diff_commit.py (Diff revisions 1 - 5)
     
     
    Show all issues

    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.

  9. reviewboard/webapi/resources/diff_commit.py (Diff revisions 1 - 5)
     
     
     
     
     
    Show all issues

    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.)

  10. reviewboard/webapi/resources/draft_diff_commit.py (Diff revisions 1 - 5)
     
     
     
    Show all issues

    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>`
    
  11. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 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. :)

  2. reviewboard/webapi/resources/diff.py (Diff revisions 5 - 6)
     
     
    Show all issues

    This will be directly rendered as ReST into the docs, so we should place double backticks around with_history and path, here and below.

  3. reviewboard/testing/testcase.py (Diff revision 6)
     
     
     
     
    Show all issues

    No blank line here.

  4. Show all issues

    Maybe "Each diff commit resource contains ..."

  5. Show all issues

    "to reproduce"

  6. Show all issues

    This might be mistaken for a hash. Maybe "The numeric ID of the commit resource."

  7. Show all issues

    "The e-mail address of ..."

    Other docs use "e-mail address," so we should be consistent there.

  8. reviewboard/webapi/resources/diff_commit.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    I wonder if we want to say "commit message" instead?

  9. Show all issues

    "DiffSets" are internal. Let's just say "on a particular commit."

  10. Show all issues

    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.
    
  11. Show all issues

    Same note about DiffSets.

  12. Show all issues

    "DiffCommit" is also internal, so let's say "Updates information on the commit."

  13. reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    Too many blank lines.

  14. Show all issues

    "resource"

  15. Show all issues

    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.

  16. Show all issues

    I'd remove "... in a DiffSet."

  17. Show all issues

    Same comment here as in the other resource.

  18. Show all issues

    "... pertaining to the diff revision." ?

  19. Show all issues

    "e-mail address"

  20. Show all issues

    "e-mail address"

  21. Show all issues

    "DiffCommit" is an internal name.

  22. Show all issues

    "DiffSet" is an internal name. Here and elsewhere in this docstring.

  23. Show all issues

    No blank line.

  24. Show all issues

    This line should no longer be needed.

  25. 
      
brennie
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
reviewbot
  1. 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
    
    
  2. reviewboard/diffviewer/forms.py (Diff revision 10)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. reviewboard/diffviewer/forms.py (Diff revision 10)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  4. reviewboard/diffviewer/forms.py (Diff revision 10)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  5. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
brennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
brennie
reviewbot
  1. 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
    
    
  2. Show all issues
     'os' imported but unused
    
  3. Show all issues
     'scmtools' imported but unused
    
  4. Show all issues
     local variable 'diff_filename' is assigned to but never used
    
  5. Show all issues
     'os' imported but unused
    
  6. Show all issues
     'scmtools' imported but unused
    
  7. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
david
  1. This is looking pretty solid!

  2. reviewboard/webapi/resources/diff.py (Diff revision 14)
     
     
    Show all issues

    This could just be if has_path == with_history

  3. Show all issues

    This should end in a period.

  4. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. Only a few very minor things left (typos, basically). All the rest of this looks very good! :D

  2. reviewboard/diffviewer/forms.py (Diff revision 15)
     
     
    Show all issues

    Missing period.

  3. Show all issues

    "commits"

  4. Show all issues

    "so that it's created empty"

  5. reviewboard/webapi/tests/test_diff_commit.py (Diff revision 15)
     
     
     
    Show all issues

    Maybe verify it is indeed one of the valid values?

  6. Show all issues

    Missing a trailing slash.

  7. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to dvcs (f224108)