Summary

Add a setting for all review bots to skip posting a review if there were no files processed.

Review Request #8192 — Created May 27, 2016 and discarded Oct. 6, 2016, 9:19 a.m.

Information

Owner

bdmartin

Repository

ReviewBot

Branch

release-0.2.x

Bugs

Depends On

Reviewers

Groups

reviewbot

People

Description

Add a setting for all review tools to skip posting a review if there were no files processed. I'm not a big fan of the variable naming, it seems confusing, but I couldn't think of better ones. Suggestions are welcome.

I've been using this for a while locally and finally polished it up a bit for review.

Testing Done

I have just the PEP8 and pyflakes bots enabled.

Setting defaults to off
- Posting a powershell file results in a review posted.
- Posting a python file results in a review posted.
Turn setting on for both bots
- Posting a powershell file results in silent bots (verified by logging).
- Posting a python file results in a review posted.

Revision 2 is currently untested as I ran in to some issues with trying to manage the settings entries in the database through django. I may need some assistance if you'd like me to test it before submission *

Issues

Description	From	Last Updated
Also, you have a typo in your testing done: "pythong"	brennie	June 10, 2016, 6:50 p.m.
Can you clean up your testing done? We have support for lists via markdown, and Verifications really doesn't belong there.	brennie	June 10, 2016, 6:51 p.m.
This should be in __init__, as it is an attribute of the instance, not the class itself. These other attributes …	brennie	June 10, 2016, 6:15 p.m.
We don't really need this here. We can pull the setting out when checking it. Also, we should call this …	brennie	June 10, 2016, 6:47 p.m.
The try should be inside the if in this configuration.	brennie	June 10, 2016, 6:36 p.m.
Format as: if (not review.ignore_irrelevant or (review.ignore_irrelevant and review.relevant_review)): Also, this is logically equivalent to: not review.ignore_irrelevant or review.relevant_review	brennie	June 10, 2016, 6:54 p.m.
We use signgle quotes for all strings.	brennie	May 27, 2016, 11:02 p.m.
Single quotes for strings.	brennie	May 27, 2016, 11:02 p.m.
Remove this.	brennie	June 10, 2016, 6:42 p.m.
Blank line between these.	brennie	June 10, 2016, 6:48 p.m.
post_trivial.	brennie	June 10, 2016, 6:48 p.m.
post_trivial and it should default to True.	brennie	June 10, 2016, 6:48 p.m.
Single quotes.	brennie	May 27, 2016, 11:02 p.m.

Tool: Pyflakes
Processed Files:
    bot/reviewbot/tasks.py
    extension/reviewbotext/evolutions/add_tool_relevant_review_setting.py
    bot/reviewbot/tools/__init__.py
    extension/reviewbotext/evolutions/__init__.py
    extension/reviewbotext/admin.py
    extension/reviewbotext/extension.py
    extension/reviewbotext/models.py
    bot/reviewbot/processing/review.py



Tool: PEP8 Style Checker
Processed Files:
    bot/reviewbot/tasks.py
    extension/reviewbotext/evolutions/add_tool_relevant_review_setting.py
    bot/reviewbot/tools/__init__.py
    extension/reviewbotext/evolutions/__init__.py
    extension/reviewbotext/admin.py
    extension/reviewbotext/extension.py
    extension/reviewbotext/models.py
    bot/reviewbot/processing/review.py

Change Summary:

Adding reviewbot group.

Groups:

+	reviewbot

People:

-	bdmartin

Fix it!

The issue has been resolved. Show all issues

Can you clean up your testing done? We have support for lists via markdown, and Verifications really doesn't belong there.

bdmartin June 23, 2016, 11:03 p.m.

Weird, when I first tried it I couldn't get the lists to indent but it worked this time.

bot/reviewbot/processing/review.py (Diff revision 1)

The issue has been resolved. Show all issues

This should be in __init__, as it is an attribute of the instance, not the class itself.
These other attributes should also be moved into __init__ but that doesn't need to be done now.
Also, can we use the term "trivial review" to refer a review with no processed files?

bot/reviewbot/processing/review.py (Diff revision 1)

The issue has been resolved. Show all issues

We don't really need this here. We can pull the setting out when checking it.

Also, we should call this post_empty_reviews.

bdmartin June 23, 2016, 11:03 p.m.

I called it post_trivial_reviews to match the trivial_review setting.

bot/reviewbot/tasks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
The try should be inside the if in this configuration.
```

bot/reviewbot/tasks.py (Diff revision 1)

The issue has been resolved. Show all issues

Format as:

if (not review.ignore_irrelevant or
    (review.ignore_irrelevant and review.relevant_review)):

Also, this is logically equivalent to:

not review.ignore_irrelevant or review.relevant_review

bdmartin June 23, 2016, 11:03 p.m.

Switched to:
if (post_trivial_reviews or not review.trivial_review):

bot/reviewbot/tasks.py (Diff revision 1)

An issue was opened. Show all issues

We use signgle quotes for all strings.

bdmartin June 23, 2016, 11:03 p.m.

Is that a new requirement? Literally every log statement in this file uses double quotes ....

I'd switch them all but I don't want to conflate the review.

bot/reviewbot/tasks.py (Diff revision 1)
An issue was opened. Show all issues
```
Single quotes for strings.
```
1. BD
  
  bdmartin June 23, 2016, 11:03 p.m.
  Same response as above.
bot/reviewbot/tasks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Remove this.
```
bot/reviewbot/tools/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```
extension/reviewbotext/admin.py (Diff revision 1)
The issue has been resolved. Show all issues
```
post_trivial.
```
1. BD
  
  bdmartin June 23, 2016, 11:03 p.m.
  Changed to post_trivial_reviews.
extension/reviewbotext/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
post_trivial and it should default to True.
```
extension/reviewbotext/models.py (Diff revision 1)
An issue was opened. Show all issues
```
Single quotes.
```
1. BD
  
  bdmartin June 23, 2016, 11:03 p.m.
  Every other help_text entry in this file uses double quotes...

The issue has been resolved. Show all issues

Also, you have a typo in your testing done: "pythong"

Testing Done:

		I have just the PEP8 and pyflakes bots enabled.

~		Verifications
	~	Setting defaults to off Posting a powershell file results in a review posted. Posting a python file results in a review posted.
	+	Turn setting on for both bots Posting a powershell file results in silent bots (verified by logging). Posting a python file results in a review posted.

~		`* Setting defaults to off`
~		* Posting a powershell file results in a review posted.
~		* Posting a python file results in a review posted.
	~
	~
	~	Revision 2 is currently untested as I ran in to some issues with trying to manage the settings entries in the database through django. I may need some assistance if you'd like me to test it before submission *
-		* Turn setting on for both bots
-		* Posting a powershell file results in silent bots (verified by logging).
-		* Posting a pythong file results in a review posted.
-

I'm working on a major reboot of Review Bot which uses the new status updates feature, which will eliminate the need for this (since we'll have feedback that a tool was run without needing to create a review).

Review Board 7.0 alpha 0 (dev)

Add a setting for all review bots to skip posting a review if there were no files processed.

Information

Reviewers

Change Summary:

Groups:

People:

Testing Done:

Verifications

Status: Discarded