flake8
passed.
JSHint
passed.
Allow ReviewBot Tools to run on review request changes without diff updates
Review Request #10173 — Created Sept. 23, 2018 and updated
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. |
 brennie
|
|
|
Docstrings should be formatted as: """Single-line summary Multi-line description. and the lines should wrap at 79. |
brennie
|
|
|
Missing a docstring explaining why its a no-op. For brevity, it can just take *args and **kwargs since all params … |
brennie
|
|
|
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
|
|
|
This is equivalent to: has_diff = diffset is not None |
brennie
|
-
-
-
-
Docstrings should be formatted as:
"""Single-line summary Multi-line description.and the lines should wrap at 79.
-
Missing a docstring explaining why its a no-op.
For brevity, it can just take
*argsand**kwargssince all params are unused. -
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_diffaround when we can just usediffset is not Nonebelow. -