Add initial web API for commit histories.

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

brennie
Review Board
dvcs
6815
6933, 6787, 6931
617622a...
reviewboard
chipx86, mike_conley, smacleod

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
  • 0
  • 0
  • 92
  • 3
  • 95
Description From Last Updated
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.  'DiffCommit' imported but unused
    
  3.  '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. Col: 1
     E302 expected 2 blank lines, found 1
    
  3. Col: 5
     E265 block comment should start with '# '
    
  4. 
      
brennie
brennie
mike_conley
  1. 
      
  2. 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...
    
  3. reviewboard/webapi/resources/diff.py (Diff revision 1)
     
     

    We also like trailing commas! :D

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

    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. 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. Nit - please add a trailing comma

  7. Nit - please add a trailing comma

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

    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. You probably didn't mean to leave this here.

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

    No blank line here.

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

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

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

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

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

    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)
     
     
     
     
     
     

    Same as above.

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

    This can be one line.

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

    No blank line.

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

    These should be listed in alphabetical order.

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

  16. Same comments on docs will apply here and elsewhere.

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

  18. This should go above the HTTP handlers.

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

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

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

  21. No blank line between try/excepts.

  22. Same here.

  23. reviewboard/webapi/server_info.py (Diff revision 1)
     
     

    This might be better suited to diffs above.

  24. Blank line here.

  25. Missing trailing slash.

  26. reviewboard/webapi/tests/test_filediff.py (Diff revision 1)
     
     
     

    Left over from something?

  27. reviewboard/webapi/tests/test_filediff.py (Diff revision 1)
     
     
     
     

    No blank line needed here

  28. 
      
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. Col: 1
     E303 too many blank lines (3)
    
  3.  '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. Col: 1
     E302 expected 2 blank lines, found 1
    
  3. 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.  '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)
     
     
     

    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)
     
     

    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)
     
     

    Missing period.

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

    Missing period.

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

    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)
     
     

    """ on the next line.

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

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

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

    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)
     
     
     
     

    No blank line here.

  4. Maybe "Each diff commit resource contains ..."

  5. "to reproduce"

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

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

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

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

  10. 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. Same note about DiffSets.

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

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

    Too many blank lines.

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

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

  16. Same comment here as in the other resource.

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

  18. "e-mail address"

  19. "e-mail address"

  20. "DiffCommit" is an internal name.

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

  22. No blank line.

  23. This line should no longer be needed.

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

    This could just be if has_path == with_history

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

    Missing period.

  3. "so that it's created empty"

  4. reviewboard/webapi/tests/test_diff_commit.py (Diff revision 15)
     
     
     

    Maybe verify it is indeed one of the valid values?

  5. Missing a trailing slash.

  6. 
      
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: Closed (submitted)

Change Summary:

Pushed to dvcs (f224108)
Loading...