Summary

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

Review Request #10173 — Created Sept. 23, 2018 and updated Sept. 23, 2018, 5:44 p.m.

Information

Owner

mike_conley

Repository

ReviewBot

Branch

master

Bugs

Depends On

Commit

9a298c0...

Reviewers

Groups

reviewbot

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
In your first sentence it should be ignore not ignores.	brennie	Sept. 23, 2018, 5:44 p.m.
Docstrings should be formatted as: """Single-line summary Multi-line description. and the lines should wrap at 79.	brennie	Sept. 23, 2018, 5:44 p.m.
Missing a docstring explaining why its a no-op. For brevity, it can just take args and *kwargs since all params …	brennie	Sept. 23, 2018, 5:44 p.m.
Doing review_request.get_latest_diffset() will at worst require a query. Instead, what if we re-architecture this like: diffset = None if (changedesc …	brennie	Sept. 23, 2018, 5:44 p.m.
This is equivalent to: has_diff = diffset is not None	brennie	Sept. 23, 2018, 5:44 p.m.

flake8 passed.

JSHint passed.

Fix it!

```
  
```

An issue was opened. Show all issues

In your first sentence it should be ignore not ignores.

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

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

An issue was opened. Show all issues

Docstrings should be formatted as:

"""Single-line summary

Multi-line description.

and the lines should wrap at 79.

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

An issue was opened. 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.

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

An issue was opened. 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.

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

An issue was opened. Show all issues

This is equivalent to:

has_diff = diffset is not None