Allow ReviewBot Tools to run on review request changes without diff updates

Review Request #10173 — Created Sept. 23, 2018 and updated

mike_conley
ReviewBot
master
9a298c0...
reviewbot

All of our ReviewBot Tools only run when a diff has been updated, and ignores
other review request updates (for example, if the summary or description is
updated).

There are some tools, however, that might want to analyze the review request
fields rather than the diff. For example, a Tool could be written to enforce
a certain style guide in the Description and Testing Done fields, or do
spell checking.

This change lets Tools opt-in to running when a review request has changed,
even if the diff wasn't updated.

I wrote a very simple ReviewBot Tool that ensures that the "Testing Done"
field has some content in it, and it successfully ran when I updated
only the Summary. It also still worked when I posted a review request
with a diff.

Description From Last Updated

In your first sentence it should be ignore not ignores.

brenniebrennie

Docstrings should be formatted as: """Single-line summary Multi-line description. and the lines should wrap at 79.

brenniebrennie

Missing a docstring explaining why its a no-op. For brevity, it can just take *args and **kwargs since all params ...

brenniebrennie

Doing review_request.get_latest_diffset() will at worst require a query. Instead, what if we re-architecture this like: diffset = None if (changedesc ...

brenniebrennie

This is equivalent to: has_diff = diffset is not None

brenniebrennie
Checks run (2 succeeded)
flake8 passed.
JSHint passed.
brennie
  1. 🎉 🎉 🎉

  2. In your first sentence it should be ignore not ignores.

  3. bot/reviewbot/tools/__init__.py (Diff revision 1)
     
     

    What about diff_required (defaulting to True) as a field name (here and below)? Just a suggestion

  4. bot/reviewbot/tools/__init__.py (Diff revision 1)
     
     
     
     

    Docstrings should be formatted as:

    """Single-line summary
    
    Multi-line description.
    

    and the lines should wrap at 79.

  5. bot/reviewbot/tools/__init__.py (Diff revision 1)
     
     
     
     

    Missing a docstring explaining why its a no-op.

    For brevity, it can just take *args and **kwargs since all params are unused.

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

    Doing review_request.get_latest_diffset() will at worst require a query. Instead, what if we re-architecture this like:

    diffset = None
    
    if (changedesc is None or
        ('diff' in changedesc.fields_changed and 
         'added' in changedesc.fields_changed['diff'])):
        diffset = review_rquest.get_latest_diffset()  
    

    This way we only retrieve the diffset when absolutely necessary, saving us some overhead.

    Along with this, I don't know that its useful to keep has_diff around when we can just use diffset is not None below.

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

    This is equivalent to:

    has_diff = diffset is not None
    
  8. 
      
Loading...