Validate commits before posting

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

brennie
RBTools
master
9911, 9920
10100
6b0d291...
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.

  • 2
  • 0
  • 13
  • 2
  • 17
Description From Last Updated
This isn't a base class. david david
typo: impement -> implement david david
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
Review request changed

Change Summary:

addressed feedback.

Commit:

-334ff183ae2f826c33e8d1d9c829bb9c9e981d9f
+6b0d2918aa2a462513e078235c8b69a7eeb45b4b

Diff:

Revision 11 (+227 -59)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
david
  1. 
      
  2. rbtools/api/resource.py (Diff revision 11)
     
     

    This isn't a base class.

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

    typo: impement -> implement

  4. 
      
Loading...