Validate commits before posting

Review Request #9921 — Created May 10, 2018 and submitted

Information

RBTools
master
4c8099f...

Reviewers

RBTools now supports the commit validation resource introduced in
/r/9920/. We now post all commits to the validation API before posting
them to the draft commit API for publishing.

Ran unit tests.
Posted this review request.
Posted /r/9609/ through to /r/9920/ to a single review request with history.

Description From Last Updated

This seems like verbose output. Do we need to show it all? If it's not near-instantaneous and we want some …

daviddavid

they -> it

daviddavid

Having both "validating" and "validated" printed to the log output seems verbose (even with debug turned on).

daviddavid

E501 line too long (89 > 79 characters)

reviewbotreviewbot

Requiring validation_info first breaks API compatibility with any existing callers of this method. It should be made optional in the …

chipx86chipx86

Alphabetical order.

chipx86chipx86

Can you pass these as keyword arguments?

chipx86chipx86

F821 undefined name 'description'

reviewbotreviewbot

Same import group.

chipx86chipx86

Why are we checking the tool name here? post should never care.

chipx86chipx86

This looks like it returns HttpRequest.

chipx86chipx86

Swap these.

chipx86chipx86

No need for parens here.

chipx86chipx86

Missing a description.

chipx86chipx86

There's some subject agreement issues here. Should this be "Validate the diffs"?

daviddavid

This isn't a base class.

daviddavid

typo: impement -> implement

daviddavid
david
  1. 
      
  2. rbtools/commands/post.py (Diff revision 1)
     
     
     

    This seems like verbose output. Do we need to show it all?

    If it's not near-instantaneous and we want some feedback, we do already pull in tqdm as a dependency, which would let us show a nifty progress bar.

  3. rbtools/commands/post.py (Diff revision 1)
     
     

    they -> it

  4. rbtools/commands/post.py (Diff revision 1)
     
     
     

    Having both "validating" and "validated" printed to the log output seems verbose (even with debug turned on).

  5. 
      
brennie
brennie
Review request changed

Change Summary:

Send parent diff with all validation and posts

Commit:

-7123760bfd3affc7ee3f9a5979ab7952e561f731
+51e0d8a8d99f297028897ce339bd02856dedb28f

Diff:

Revision 3 (+190 -55)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
chipx86
  1. 
      
  2. rbtools/api/resource.py (Diff revision 5)
     
     
     
     
     

    Requiring validation_info first breaks API compatibility with any existing callers of this method. It should be made optional in the function signature.

    This also needs to be added to the docstring.

    1. Scratch that, I read the class name wrong and thought this was a released resource.

    2. (So this is now just about the docstring.)

  3. rbtools/commands/post.py (Diff revision 5)
     
     
     

    Alphabetical order.

  4. rbtools/commands/post.py (Diff revision 5)
     
     
     
     
     
     
     
     
     

    Can you pass these as keyword arguments?

  5. 
      
brennie
Review request changed

Change Summary:

Addressed issues & refactored calls to tqdm

Commit:

-a749d2e89a4efb6a6639865bb4f5b23b75153aed
+177650dde6fd640861aa7e616e2ccae187ba2b8e

Diff:

Revision 6 (+223 -58)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
chipx86
  1. 
      
  2. rbtools/commands/post.py (Diff revision 8)
     
     
     
     
     

    Same import group.

  3. rbtools/commands/post.py (Diff revision 8)
     
     

    Why are we checking the tool name here? post should never care.

    1. This is moved code.

  4. 
      
brennie
chipx86
  1. 
      
  2. rbtools/api/resource.py (Diff revision 9)
     
     
     

    This looks like it returns HttpRequest.

    1. The @request_method_decorator takes an HttpRequest returning function, executes that request, and transforms it into the appropriate resource.

  3. rbtools/commands/post.py (Diff revision 9)
     
     
     

    Swap these.

  4. rbtools/commands/post.py (Diff revision 9)
     
     

    No need for parens here.

  5. rbtools/commands/post.py (Diff revision 9)
     
     
     

    Missing a description.

  6. 
      
brennie
david
  1. 
      
  2. rbtools/commands/post.py (Diff revision 10)
     
     

    There's some subject agreement issues here. Should this be "Validate the diffs"?

  3. 
      
brennie
david
  1. 
      
  2. rbtools/api/resource.py (Diff revision 11)
     
     

    This isn't a base class.

    1. Every resource is like this.

  3. rbtools/commands/post.py (Diff revision 11)
     
     

    typo: impement -> implement

  4. 
      
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (b898a1e)
Loading...