Validate commits before posting
Review Request #9921 — Created May 9, 2018 and submitted
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 … |
david | |
they -> it |
david | |
Having both "validating" and "validated" printed to the log output seems verbose (even with debug turned on). |
david | |
E501 line too long (89 > 79 characters) |
reviewbot | |
Requiring validation_info first breaks API compatibility with any existing callers of this method. It should be made optional in the … |
chipx86 | |
Alphabetical order. |
chipx86 | |
Can you pass these as keyword arguments? |
chipx86 | |
F821 undefined name 'description' |
reviewbot | |
Same import group. |
chipx86 | |
Why are we checking the tool name here? post should never care. |
chipx86 | |
This looks like it returns HttpRequest. |
chipx86 | |
Swap these. |
chipx86 | |
No need for parens here. |
chipx86 | |
Missing a description. |
chipx86 | |
There's some subject agreement issues here. Should this be "Validate the diffs"? |
david | |
This isn't a base class. |
david | |
typo: impement -> implement |
david |
-
-
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.
-
-
rbtools/commands/post.py (Diff revision 1) 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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+189 -54) |
Checks run (2 succeeded)
Change Summary:
Send parent diff with all validation and posts
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+190 -55) |
Checks run (1 failed, 1 succeeded)
flake8
Depends On: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 4 (+189 -54) |
Checks run (2 succeeded)
Change Summary:
Only send validation info if present.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+191 -54) |
Checks run (2 succeeded)
-
-
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.
-
-
Change Summary:
Addressed issues & refactored calls to tqdm
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+223 -58) |
Checks run (1 failed, 1 succeeded)
flake8
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+223 -58) |
Checks run (2 succeeded)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+223 -59) |
Checks run (2 succeeded)
-
-
-
rbtools/commands/post.py (Diff revision 8) Why are we checking the tool name here?
post
should never care.
Change Summary:
Addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+224 -61) |
Checks run (2 succeeded)
Change Summary:
addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+224 -59) |
Checks run (2 succeeded)
-
-
rbtools/commands/post.py (Diff revision 10) There's some subject agreement issues here. Should this be "Validate the diffs"?
Change Summary:
addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+227 -59) |
Checks run (2 succeeded)
Change Summary:
addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+227 -59) |