`rbt post` command uses API's DiffValidation endpoint before upload

Review Request #6407 — Created Oct. 6, 2014 and submitted

Information

RBTools
master
a288b2c...

Reviewers

Before updating a review request or creating a new one, `rbt post`
command validates the diff file and terminates if it does not pass
validation. This prevents the command from creating empty review
requests when the supplied diff file is invalid.

Tested rbt post with:
1. Empty diff (error 219)
2. Diff from a different repository (error 207)
3. Non-diff file as a diff (error 224)
4. Valid diff (success)

using:
1. Git
2. Mercurial

Also tested base_dir with a subversion reporitory.

Description From Last Updated

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 11 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 11 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 11 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 11 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 11 E111 indentation is not a multiple of four

reviewbotreviewbot

Can you move this to come after DiffResource? You just stuck it in between DiffListResource and DiffResource.

daviddavid

The resource isn't actually a "List", it's just a bug in the API docs. Let's just document it here as …

SM smacleod

This should have a comment mentioning the TODO of unifying this and DiffListResource.update_diff (and probably a correspending comment in update_diff)

daviddavid

You should avoid breaking lines with \\ whenever possible. This should probably be written: RESOURCE_MAP['application/vnd.reviewboard.org.diff-validation'] = ( DiffValidationResource)

SM smacleod

You need to pass in the parent diff and base dir, if they exist. Parent diffs can be tested by …

daviddavid

Can you make sure that everything here uses single quotes?

daviddavid

Can you use single quotes for these strings?

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/api/resource.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/api/resource.py
    
    
  2. rbtools/api/resource.py (Diff revision 1)
     
     
    Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  3. rbtools/commands/post.py (Diff revision 1)
     
     
    Show all issues
    Col: 11
     E111 indentation is not a multiple of four
    
  4. rbtools/commands/post.py (Diff revision 1)
     
     
    Show all issues
    Col: 11
     E111 indentation is not a multiple of four
    
  5. rbtools/commands/post.py (Diff revision 1)
     
     
    Show all issues
    Col: 11
     E111 indentation is not a multiple of four
    
  6. rbtools/commands/post.py (Diff revision 1)
     
     
    Show all issues
    Col: 11
     E111 indentation is not a multiple of four
    
  7. rbtools/commands/post.py (Diff revision 1)
     
     
    Show all issues
    Col: 11
     E111 indentation is not a multiple of four
    
  8. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/api/resource.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/api/resource.py
    
    
  2. 
      
david
  1. If this is more or less complete, you should remove the 'WIP' tag from the summary.

    I'd like to see more testing with additional version control systems (and some testing of parent diffs).

  2. rbtools/api/resource.py (Diff revision 2)
     
     
    Show all issues

    Can you move this to come after DiffResource? You just stuck it in between DiffListResource and DiffResource.

    1. I moved it to the end of file, since ValidateDiff resource comes last in Resource Tree page of the documentation. https://www.reviewboard.org/docs/manual/dev/webapi/2.0/resources/resource-tree/

  3. rbtools/api/resource.py (Diff revision 2)
     
     
    Show all issues

    This should have a comment mentioning the TODO of unifying this and DiffListResource.update_diff (and probably a correspending comment in update_diff)

  4. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues

    You need to pass in the parent diff and base dir, if they exist.

    Parent diffs can be tested by making two commits on git, and then running 'rbt post HEAD' with the latest commit checked out. The base dir can be tested by connecting an SVN repository and posting a change against it.

  5. 
      
SM
  1. 
      
  2. rbtools/api/resource.py (Diff revision 2)
     
     
    Show all issues

    The resource isn't actually a "List", it's just a bug in the API docs. Let's just document it here as The Validate Diff resource specific base class.

  3. rbtools/api/resource.py (Diff revision 2)
     
     
     
    Show all issues

    You should avoid breaking lines with \\ whenever possible. This should probably be written:
    RESOURCE_MAP['application/vnd.reviewboard.org.diff-validation'] = (
    DiffValidationResource)

    1. Actually, when it's just a single line that's wrapping like this, we do use \.

    2. I copied this style from other similar lines in that file. I'm guessing I should drop this issue.

  4. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/api/resource.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/api/resource.py
    
    
  2. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/api/resource.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/api/resource.py
    
    
  2. 
      
AS
david
  1. 
      
  2. rbtools/api/resource.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
    Show all issues

    Can you make sure that everything here uses single quotes?

  3. rbtools/commands/post.py (Diff revision 4)
     
     
     
     
     
     
     
    Show all issues

    Can you use single quotes for these strings?

  4. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/api/resource.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/api/resource.py
    
    
  2. 
      
dkus
  1. Could you add the group 'students' to the reviewers?

  2. 
      
AS
david
  1. Ship It!

  2. 
      
AS
Review request changed
Status:
Completed
Change Summary:
Pushed to release-0.6.x (9b18656)