flake8
passed.
JSHint
passed.
Support running tools manually
Review Request #10316 — Created Nov. 8, 2018 and discarded
Support running tools attached to status updates manually, along with
retrying status updates in an error state.Adds a new signal hook for the manual run feature that will update
previous status updates that have not been run yet or need a rerun.
Manual testing has been done (waiting on testing framework). By adding
multiple integrations for different types of bots, running some
manually, and retrying some.
| Description | From | Last Updated |
|---|---|---|
|
Can you rewrite your summary in the imperitive mood, i.e. as a command? If you substitute your summary into the … |
 brennie
|
|
|
Looks good to me and reads well. |
 gojeffcho
|
|
|
This needs a a help_text= kwarg to describe what exactly this means. Maybe we should call this manually_run_only and change … |
brennie
|
|
|
This should go at the top of the function and there should be a blank line after. |
brennie
|
|
|
E303 too many blank lines (2) |
 reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
Can we call this _get_maching_configs ? We call them configurations in the docstring. |
brennie
|
|
|
Format as: Yields: tuple: A 3-tuple of the following: * The tool model to use (:py:class:`reviewbotext.models.Tool`). * The tool options … |
brennie
|
|
|
Can you format as: for tool, tool_options, review_settings in self._get_matching_setups( review_request): # ... You can also pull out the function … |
brennie
|
|
|
No blank line. |
brennie
|
|
|
Same comment as above. |
brennie
|
|
|
Let's put this at the end of the tuple. I expect it to be used less often than the other … |
 david
|
|
|
Lets just say "configurations" instead of "tool, review configs" |
brennie
|
|
|
All the other descriptions are lower case. Do we want this to be too? Also, shouldn't this (and all other … |
brennie
|
|
|
same here re: translation. If we end up using translated descriptions and we are using them in multiple places, they … |
brennie
|
- Change Summary:
-
Update timestamp when running/rerunning tool
- Commit:
-
b167436389cf745d4b5f7de5a796461bda7cbd21a75bcfd93b5ccb2480bc371f95b5fb1284063a01
- Change Summary:
-
Removed WIP tag, attmpted to clean up duplicate code with a new function that generates valid setups.
- Summary:
-
[WIP] Initial addition of ReviewBot manual run featureInitial addition of ReviewBot manual run feature
- Description:
-
~ Initial addition of manual run feature
~ Added manual run option to integration configurations, which allows for
+ manual run instead of tools running on review request updates. ~ Not looking for feedback yet.
~ (aside from high-level design direction) ~ Added new signal hook for manual run feature that will update previous
~ status updates that have not been run yet or need a rerun. ~ Duplicate code will be cleaned up.
~ Cleaned up code by adding a generator for matching tool setups.
+ + Some improvements could be made to the
_get_matching_setups()function. - Testing Done:
-
+ Manual testing has been done (waiting on testing framework). By adding
+ multiple integrations for different types of bots, running some + manually, and retrying some. - Commit:
-
a75bcfd93b5ccb2480bc371f95b5fb1284063a0148ac68c658b71538fcb035bd67c873ce33b55d61
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
Can you rewrite your summary in the imperitive mood, i.e. as a command? If you substitute your summary into the following template, it should be a correct sentence.
This patch will <summary>e.g. "Support running tools manually"
-
-
Format as:
Yields: tuple: A 3-tuple of the following: * The tool model to use (:py:class:`reviewbotext.models.Tool`). * The tool options (:py:class:`dict`). * The review settings (:py:class:`dict`).Could you also elaborate on what
review_settingsactually is in that? -
Can you format as:
for tool, tool_options, review_settings in self._get_matching_setups( review_request): # ...You can also pull out the function call into a var and iterate over that, e.g.
setups = self._get_matching_setups(review_request) for a, b, c in setups: # ... -
-
- Change Summary:
-
Updated description and documentation wording, moved missing configuration check into main code, minor formatting changes.
- Description:
-
~ Added manual run option to integration configurations, which allows for
~ manual run instead of tools running on review request updates. ~ Support running tools attached to status updates manually, along with
~ retrying status updates in an error state. ~ Added new signal hook for manual run feature that will update previous
~ status updates that have not been run yet or need a rerun. ~ Adds a new signal hook for the manual run feature that will update
~ previous status updates that have not been run yet or need a rerun. - - Cleaned up code by adding a generator for matching tool setups.
- - Some improvements could be made to the
_get_matching_setups()function. - Commit:
-
48ac68c658b71538fcb035bd67c873ce33b55d61268ebe6c0fcd791a99bec563048b53d726bb7749
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Reordered tool options in form
- Commit:
-
268ebe6c0fcd791a99bec563048b53d726bb77490f7a3d936a3cf162b5151002ac51003eed451ca0
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
All the other descriptions are lower case. Do we want this to be too?
Also, shouldn't this (and all other descriptions) be marked for translation?
-
same here re: translation.
If we end up using translated descriptions and we are using them in multiple places, they should be constants somewhere so they stay in sync with one another
- Change Summary:
-
Made descriptions translatable, used constants to maintain consistency, fixed capitalization.
- Commit:
-
0f7a3d936a3cf162b5151002ac51003eed451ca0ad1bddbdf37384d2c250f29474ee98376479ee2d
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
