Summary:

Validate commits before posting

Review Request #9921 — Created May 9, 2018 and submitted Oct. 23, 2018, 12:43 p.m.

Information
Owner:	brennie
Repository:	RBTools
Branch:	master
Bugs:
Depends On:	~~9911~~, ~~9920~~
Blocks:	10240, 10100
Commit:	4c8099f...
Reviewers
Groups:	rbtools
People:

Description

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.

Testing Done

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

Issues

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 …	david	July 11, 2018, 4:32 p.m.
they -> it	david	July 5, 2018, 4:12 p.m.
Having both "validating" and "validated" printed to the log output seems verbose (even with debug turned on).	david	July 5, 2018, 4:15 p.m.
E501 line too long (89 > 79 characters)	reviewbot	Aug. 16, 2018, 8:08 p.m.
Requiring validation_info first breaks API compatibility with any existing callers of this method. It should be made optional in the …	chipx86	Aug. 20, 2018, 1:13 p.m.
Alphabetical order.	chipx86	Aug. 20, 2018, 1:13 p.m.
Can you pass these as keyword arguments?	chipx86	Aug. 20, 2018, 1:14 p.m.
F821 undefined name 'description'	reviewbot	Aug. 20, 2018, 1:35 p.m.
Same import group.	chipx86	Aug. 30, 2018, 2:06 p.m.
Why are we checking the tool name here? post should never care.	chipx86	Aug. 29, 2018, 7:23 p.m.
This looks like it returns HttpRequest.	chipx86	Sept. 4, 2018, 12:27 p.m.
Swap these.	chipx86	Sept. 4, 2018, 12:26 p.m.
No need for parens here.	chipx86	Sept. 4, 2018, 12:26 p.m.
Missing a description.	chipx86	Sept. 4, 2018, 12:27 p.m.
There's some subject agreement issues here. Should this be "Validate the diffs"?	david	Sept. 27, 2018, 9:12 a.m.
This isn't a base class.	david	Oct. 18, 2018, 10:07 p.m.
typo: impement -> implement	david	Oct. 18, 2018, 10:07 p.m.

flake8 passed.

JSHint passed.

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

Show all issues

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.

rbtools/commands/post.py (Diff revision 1)
Show all issues
```
they -> it
```

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

Show all issues

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

Change Summary:

Use TQDM to show a progress bar.

Commit:

-	a3f6fe70e98b5d68eda1ce04638b00b431937378
+	7123760bfd3affc7ee3f9a5979ab7952e561f731

Diff:

Revision 2 (+189 -54)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Send parent diff with all validation and posts

Commit:

-	7123760bfd3affc7ee3f9a5979ab7952e561f731
+	51e0d8a8d99f297028897ce339bd02856dedb28f

Diff:

Revision 3 (+190 -55)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/post.py (Diff revision 3)
Show all issues
```
E501 line too long (89 > 79 characters)
```

Depends On:

+	~~9911 - Post review requests with history when supported~~

Commit:

-	51e0d8a8d99f297028897ce339bd02856dedb28f
+	c2b264eb2df28cd49374f39cce4db13ec525bb49

Diff:

Revision 4 (+189 -54)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Only send validation info if present.

Commit:

-	c2b264eb2df28cd49374f39cce4db13ec525bb49
+	a749d2e89a4efb6a6639865bb4f5b23b75153aed

Diff:

Revision 5 (+191 -54)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/api/resource.py (Diff revision 5)

Show all issues

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.

chipx86 Aug. 17, 2018, 4:11 p.m.

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

chipx86 Aug. 17, 2018, 4:11 p.m.

(So this is now just about the docstring.)

rbtools/commands/post.py (Diff revision 5)
Show all issues
```
Alphabetical order.
```

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

Show all issues

Can you pass these as keyword arguments?

Change Summary:

Addressed issues & refactored calls to tqdm

Commit:

-	a749d2e89a4efb6a6639865bb4f5b23b75153aed
+	177650dde6fd640861aa7e616e2ccae187ba2b8e

Diff:

Revision 6 (+223 -58)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/post.py (Diff revision 6)
Show all issues
```
F821 undefined name 'description'
```

Commit:

-	177650dde6fd640861aa7e616e2ccae187ba2b8e
+	91d5d2a5b2459eb381c2e550400b42232271b089

Diff:

Revision 7 (+223 -58)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	91d5d2a5b2459eb381c2e550400b42232271b089
+	6756dc6e310c8fde9ea23029b37570c659d57a6f

Diff:

Revision 8 (+223 -59)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/commands/post.py (Diff revision 8)
Show all issues
```
Same import group.
```

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

Show all issues

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

brennie Aug. 29, 2018, 7:23 p.m.
```
This is moved code.
```

Change Summary:

Addressed feedback.

Commit:

-	6756dc6e310c8fde9ea23029b37570c659d57a6f
+	df6ddcdc8673533203816ad2c73d012f5ba096bb

Diff:

Revision 9 (+224 -61)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/api/resource.py (Diff revision 9)

Show all issues

This looks like it returns HttpRequest.

brennie Sept. 4, 2018, 12:28 p.m.

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

rbtools/commands/post.py (Diff revision 9)
Show all issues
```
Swap these.
```
rbtools/commands/post.py (Diff revision 9)
Show all issues
```
No need for parens here.
```
rbtools/commands/post.py (Diff revision 9)
Show all issues
```
Missing a description.
```

Change Summary:

addressed feedback.

Commit:

-	df6ddcdc8673533203816ad2c73d012f5ba096bb
+	334ff183ae2f826c33e8d1d9c829bb9c9e981d9f

Diff:

Revision 10 (+224 -59)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/commands/post.py (Diff revision 10)

Show all issues

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

Change Summary:

addressed feedback.

Commit:

-	334ff183ae2f826c33e8d1d9c829bb9c9e981d9f
+	6b0d2918aa2a462513e078235c8b69a7eeb45b4b

Diff:

Revision 11 (+227 -59)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/api/resource.py (Diff revision 11)
Show all issues
```
This isn't a base class.
```
1. brennie Oct. 18, 2018, 10:06 p.m.
  Every resource is like this.
rbtools/commands/post.py (Diff revision 11)
Show all issues
```
typo: impement -> implement
```

Change Summary:

addressed feedback.

Commit:

-	6b0d2918aa2a462513e078235c8b69a7eeb45b4b
+	4c8099f48f4edd74b9867d23063c5c2e9d5e3165

Diff:

Revision 12 (+227 -59)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (b898a1e)