flake8
passed.
JSHint
passed.
Add status-update command to rbtools.
Review Request #9343 — Created Nov. 1, 2017 and submitted
Review Board 3.0 introduced new support for "status updates" which are
used to indicate if a review request is undergoing some automated checks
of some kind (for instance, automated code review or pending builds).
These can have a state (such as "pending", "error", "success", and
others), a summary, optional link, and a review (when there are errors).This adds a new command for working with status updates, helping to
create/update the state of a status update. This command can be run with
rbt status-update.
Debug testing of the
--reviewinput on local installation with the following file contents:{ "review": { "body_top": "Header comment" }, "diff_comment": [ { "filediff_id": 10, "first_line": 729, "issue_opened": true, "num_lines": 1, "text": "Adding a comment on a diff line", "text_type": "markdown" } ], "general_comments": [ { "text": "Adding a general comment", "text_type": "markdown" } ] }
| Description | From | Last Updated |
|---|---|---|
|
Missing docs rst file for new status-update command. |
 bleblan2
|
|
|
You might want to pull out each action into its own function. |
 brennie
|
|
|
I know most files in rbtools don't have them (yet), but we should have a file docstring at the top. |
 david
|
|
|
Add a blank line between these two. |
david
|
|
|
Needs "Args" and "Returns" sections. |
david
|
|
|
We don't need to define a variable if we're immediately returning it. It would also be a bit nicer to … |
david
|
|
|
Needs an "Args" section. |
david
|
|
|
Needs "Args" and "Returns" sections. |
david
|
|
|
Needs "Args" and "Raises" sections. |
david
|
|
|
Needs "Args" and "Raises" sections. |
david
|
|
|
It's kind of confusing how this can either be an individual status update or the list of all status updates. … |
david
|
|
|
Needs "Args" and "Raises" sections. |
david
|
|
|
Needs "Args" and "Raises" sections. |
david
|
|
|
In addition to specifying the details like this, perhaps we can show an example JSON blob? |
david
|
|
|
paren can go on the previous line. |
david
|
|
|
Instead of using the continuation character, let's wrap the whole thing in parens: if (not file_contents.get('reviews') and not file_contents.get('diff_comments') and … |
david
|
|
|
get -> hasattr |
david
|
|
|
get -> hasattr |
david
|
|
|
get -> hasattr |
david
|
|
|
Needs a period. |
david
|
|
|
Needs "Args" and "Raises" sections. |
david
|
|
|
Needs a period. |
david
|
|
|
I'm guessing you haven't yet tested this functionality because you'll need to associate the newly created review ID with the … |
david
|
|
|
mode is also unicode |
david
|
|
|
mode is also unicode, but we shouldn't need to have it as an arg at all because we'll always be … |
david
|
|
|
You can avoid having the entire get_file function by doing: return json.load(open(path, 'r')) We shouldn't need expandvars or expanduser because … |
david
|
|
|
F841 local variable 'required_fields' is assigned to but never used |
 reviewbot
|
- Change Summary:
-
Move each action into their own functions.
- Commit:
-
0514de804b51b2e307e0c98fb7def0e30814db67757a8df6f2f4f748f3592e03f3d460afe8a2b904
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Add current dev changes.
- Commit:
-
757a8df6f2f4f748f3592e03f3d460afe8a2b9044ac4f48cb547d6069cedc5d8886be022cdfba9f7
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Fix valid state options.
- Commit:
-
4ac4f48cb547d6069cedc5d8886be022cdfba9f78908fce6c2533ac34266d530a6dfc723ceddff67
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Add basic review creation functionality.
- Testing Done:
-
+ Debug testing of the
--reviewinput on local installation with the following file contents:+ + + + {+ "review": {+ "body_top": "Header comment"
+ },
+ "diff_comment": [
+ {+ "filediff_id": 10,
+ "first_line": 729,
+ "issue_opened": true,
+ "num_lines": 1,
+ "text": "Adding a comment on a diff line",
+ "text_type": "markdown"
+ }
+ ],
+ "general_comments": [
+ {+ "text": "Adding a general comment",
+ "text_type": "markdown"
+ }
+ ]
+ }
+ - Commit:
-
8908fce6c2533ac34266d530a6dfc723ceddff67bf25b22d2a0b042ec2edac72ef53615d4f756a5f
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
-
-
We don't need to define a variable if we're immediately returning it. It would also be a bit nicer to wrap this as:
return { key: status_update.get(key) for key in keys if status_update.get(key) } -
-
-
-
-
It's kind of confusing how this can either be an individual status update or the list of all status updates. I think I'd prefer if the conditional (do we have
self.options.sid) is the top-level construct and then we either update or create as appropriate. -
-
-
-
-
Instead of using the continuation character, let's wrap the whole thing in parens:
if (not file_contents.get('reviews') and not file_contents.get('diff_comments') and not file_contents.get('general_comments')):You can also use
hasattrinstead ofget, which is a bit more efficient. -
-
-
-
-
-
-
I'm guessing you haven't yet tested this functionality because you'll need to associate the newly created review ID with the status update.
It's also a good idea to do that association before publishing the review, so it doesn't show up (momentarily) as a regular review.
-
-
modeis alsounicode, but we shouldn't need to have it as an arg at all because we'll always be reading the file. We can just pass in'r'toopen. -
You can avoid having the entire
get_filefunction by doing:return json.load(open(path, 'r'))We shouldn't need
expandvarsorexpanduserbecause the user's shell should have done the globbing for us.
- Change Summary:
-
Update review request based on feedback and clean stuff up.
- Commit:
-
bf25b22d2a0b042ec2edac72ef53615d4f756a5f1bf3650aa598314f893a130839e449ab61654ea9
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Add attaching of created review to status-update and move out of WIP.
- Summary:
-
[WIP] Add status-update command to rbtools.Add status-update command to rbtools.
- Commit:
-
1bf3650aa598314f893a130839e449ab61654ea95d28a093765d80ed303bd112795b1b8378fd2733
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Add docs rst file for status-update and add APIError catching to review creation.
- Commit:
-
5d28a093765d80ed303bd112795b1b8378fd2733640828e7cd8cb61e016991b922cc6674b3fdce6f