This change has been marked as completed.

Describe the completed change (optional):

Pushed to master (b898a1e)

Summary

Validate commits before posting

Review Request #9921 — Created May 9, 2018 and submitted 6 years, 6 months ago

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	6 years, 9 months ago
they -> it	david	6 years, 10 months ago
Having both "validating" and "validated" printed to the log output seems verbose (even with debug turned on).	david	6 years, 10 months ago
E501 line too long (89 > 79 characters)	reviewbot	6 years, 8 months ago
Requiring validation_info first breaks API compatibility with any existing callers of this method. It should be made optional in the …	chipx86	6 years, 8 months ago
Alphabetical order.	chipx86	6 years, 8 months ago
Can you pass these as keyword arguments?	chipx86	6 years, 8 months ago
F821 undefined name 'description'	reviewbot	6 years, 8 months ago
Same import group.	chipx86	6 years, 8 months ago
Why are we checking the tool name here? post should never care.	chipx86	6 years, 8 months ago
This looks like it returns HttpRequest.	chipx86	6 years, 8 months ago
Swap these.	chipx86	6 years, 8 months ago
No need for parens here.	chipx86	6 years, 8 months ago
Missing a description.	chipx86	6 years, 8 months ago
There's some subject agreement issues here. Should this be "Validate the diffs"?	david	6 years, 7 months ago
This isn't a base class.	david	6 years, 6 months ago
typo: impement -> implement	david	6 years, 6 months ago
There are no open issues

flake8 passed.

JSHint passed.

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

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
they -> it
```

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

The issue has been resolved. 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)
The issue has been resolved. 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)

The issue has been resolved. 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 6 years, 8 months ago

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

chipx86 6 years, 8 months ago

(So this is now just about the docstring.)

rbtools/commands/post.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Alphabetical order.
```

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

The issue has been resolved. 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)
The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
Same import group.
```
rbtools/commands/post.py (Diff revision 8)
The issue has been dropped. Show all issues
```
Why are we checking the tool name here? post should never care.
```
1. brennie 6 years, 8 months ago
  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)

The issue has been dropped. Show all issues

This looks like it returns HttpRequest.

brennie 6 years, 8 months ago

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)
The issue has been resolved. Show all issues
```
Swap these.
```
rbtools/commands/post.py (Diff revision 9)
The issue has been resolved. Show all issues
```
No need for parens here.
```
rbtools/commands/post.py (Diff revision 9)
The issue has been resolved. 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)

The issue has been resolved. 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)
The issue has been dropped. Show all issues
```
This isn't a base class.
```
1. brennie 6 years, 6 months ago
  Every resource is like this.
rbtools/commands/post.py (Diff revision 11)
The issue has been resolved. 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:: Completed
Change Summary:: Pushed to master (b898a1e)