Validate commits before posting

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

brennie
RBTools
master
9911, 9920
10240, 10100
4c8099f...
rbtools

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.

  • 0
  • 0
  • 14
  • 3
  • 17
Description From Last Updated
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...