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

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

Information

ReviewBot
master
9a298c0...

Reviewers

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. Show all issues

    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)
     
     
     
     
    Show all issues

    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)
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
    Show all issues

    This is equivalent to:

    has_diff = diffset is not None
    
  8.