Support running tools manually

Review Request #10316 — Created Nov. 8, 2018 and updated

alextechcc
ReviewBot
master
10311
e9a7412...
reviewbot, students

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 ...

brenniebrennie

Looks good to me and reads well.

gojeffchogojeffcho

This needs a a help_text= kwarg to describe what exactly this means. Maybe we should call this manually_run_only and change ...

brenniebrennie

This should go at the top of the function and there should be a blank line after.

brenniebrennie

E303 too many blank lines (2)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

Can we call this _get_maching_configs ? We call them configurations in the docstring.

brenniebrennie

Format as: Yields: tuple: A 3-tuple of the following: * The tool model to use (:py:class:`reviewbotext.models.Tool`). * The tool options ...

brenniebrennie

Can you format as: for tool, tool_options, review_settings in self._get_matching_setups( review_request): # ... You can also pull out the function ...

brenniebrennie

No blank line.

brenniebrennie

Same comment as above.

brenniebrennie

Let's put this at the end of the tuple. I expect it to be used less often than the other ...

daviddavid

Lets just say "configurations" instead of "tool, review configs"

brenniebrennie

All the other descriptions are lower case. Do we want this to be too? Also, shouldn't this (and all other ...

brenniebrennie

same here re: translation. If we end up using translated descriptions and we are using them in multiple places, they ...

brenniebrennie
brennie
  1. 
      
  2. extension/reviewbotext/forms.py (Diff revision 1)
     
     

    This needs a a help_text= kwarg to describe what exactly this means.

    Maybe we should call this manually_run_only and change the label to "Only run this tool manually" or similar?

  3. extension/reviewbotext/integration.py (Diff revision 1)
     
     

    This should go at the top of the function and there should be a blank line after.

  4. 
      
alextechcc
Review request changed

Commit:

-11e5205ffbcb711b0e4669b988dfbec3d434fc9e
+b167436389cf745d4b5f7de5a796461bda7cbd21

Diff:

Revision 2 (+137 -11)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

alextechcc
Review request changed

Change Summary:

Update timestamp when running/rerunning tool

Commit:

-b167436389cf745d4b5f7de5a796461bda7cbd21
+a75bcfd93b5ccb2480bc371f95b5fb1284063a01

Diff:

Revision 3 (+139 -11)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

alextechcc
brennie
  1. 
      
  2. 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"

  3. extension/reviewbotext/integration.py (Diff revision 4)
     
     

    Can we call this _get_maching_configs ? We call them configurations in the docstring.

  4. extension/reviewbotext/integration.py (Diff revision 4)
     
     
     
     
     
     
     

    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_settings actually is in that?

  5. extension/reviewbotext/integration.py (Diff revision 4)
     
     
     

    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:
        # ...
    
  6. extension/reviewbotext/integration.py (Diff revision 4)
     
     

    No blank line.

  7. extension/reviewbotext/integration.py (Diff revision 4)
     
     
     

    Same comment as above.

  8. 
      
alextechcc
david
  1. 
      
  2. extension/reviewbotext/forms.py (Diff revision 5)
     
     

    Let's put this at the end of the tuple. I expect it to be used less often than the other options.

  3. 
      
alextechcc
alextechcc
gojeffcho
  1. 
      
  2. Looks good to me and reads well.

  3. 
      
brennie
  1. 
      
  2. extension/reviewbotext/integration.py (Diff revision 6)
     
     

    Lets just say "configurations" instead of "tool, review configs"

  3. extension/reviewbotext/integration.py (Diff revision 6)
     
     

    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?

  4. extension/reviewbotext/integration.py (Diff revision 6)
     
     

    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

  5. 
      
alextechcc
alextechcc
Review request changed

Change Summary:

Mirroring changes in other review request

Commit:

-ad1bddbdf37384d2c250f29474ee98376479ee2d
+e9a741274e189c8f4dc18fae4934e2a46a7c11ae

Diff:

Revision 8 (+168 -47)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...