Merge pull request #423 from zmeir/zmeir-external-incremental_review_thresholds

Implementing Thresholds for Incremental PR Reviews

docs/REVIEW.md

@@ -29,12 +29,26 @@ Under the section 'pr_reviewer', the [configuration file](./../pr_agent/settings
 ####  Incremental Mode
 For an incremental review, which only considers changes since the last PR-Agent review, this can be useful when working on the PR in an iterative manner, and you want to focus on the changes since the last review instead of reviewing the entire PR again, the following command can be used:
 ```
-/improve -i
+/review -i
 ```
 Note that the incremental mode is only available for GitHub.
 
 <kbd><img src=./../pics/incremental_review.png width="768"></kbd>
 
+Under the section 'pr_reviewer', the [configuration file](./../pr_agent/settings/configuration.toml#L16) contains options to customize the 'review -i' tool.  
+These configurations can be used to control the rate at which the incremental review tool will create new review comments when invoked automatically, to prevent making too much noise in the PR.
+- `minimal_commits_for_incremental_review`: Minimal number of commits since the last review that are required to create incremental review.
+If there are less than the specified number of commits since the last review, the tool will not perform any action.
+Default is 0 - the tool will always run, no matter how many commits since the last review.
+- `minimal_minutes_for_incremental_review`: Minimal number of minutes that need to pass since the last reviewed commit to create incremental review.
+If less that the specified number of minutes have passed between the last reviewed commit and running this command, the tool will not perform any action. 
+Default is 0 - the tool will always run, no matter how much time have passed since the last reviewed commit.
+- `require_all_thresholds_for_incremental_review`: If set to true, all the previous thresholds must be met for incremental review to run. If false, only one is enough to run the tool.
+For example, if `minimal_commits_for_incremental_review=2` and `minimal_minutes_for_incremental_review=2`, and we have 3 commits since the last review, but the last reviewed commit is from 1 minute ago:
+When `require_all_thresholds_for_incremental_review=true` the incremental review __will not__ run, because only 1 out of 2 conditions were met (we have enough commits but the last review is too recent),
+but when `require_all_thresholds_for_incremental_review=false` the incremental review __will__ run, because one condition is enough (we have 3 commits which is more than the configured 2).
+Default is false - the tool will run as long as at least once conditions is met.
+
 #### PR Reflection
 By invoking:
 ```

pr_agent/git_providers/git_provider.py

@@ -190,6 +190,13 @@ class IncrementalPR:
     def __init__(self, is_incremental: bool = False):
         self.is_incremental = is_incremental
         self.commits_range = None
-        self.first_new_commit_sha = None
-        self.last_seen_commit_sha = None
+        self.first_new_commit = None
+        self.last_seen_commit = None
 
+    @property
+    def first_new_commit_sha(self):
+        return None if self.first_new_commit is None else self.first_new_commit.sha
+
+    @property
+    def last_seen_commit_sha(self):
+        return None if self.last_seen_commit is None else self.last_seen_commit.sha

pr_agent/git_providers/github_provider.py

@@ -66,10 +66,10 @@ class GithubProvider(GitProvider):
         first_new_commit_index = None
         for index in range(len(self.commits) - 1, -1, -1):
             if self.commits[index].commit.author.date > last_review_time:
-                self.incremental.first_new_commit_sha = self.commits[index].sha
+                self.incremental.first_new_commit = self.commits[index]
                 first_new_commit_index = index
             else:
-                self.incremental.last_seen_commit_sha = self.commits[index].sha
+                self.incremental.last_seen_commit = self.commits[index]
                 break
         return self.commits[first_new_commit_index:] if first_new_commit_index is not None else []
 

pr_agent/settings/configuration.toml

@@ -26,6 +26,10 @@ ask_and_reflect=false
 automatic_review=true
 remove_previous_review_comment=false
 extra_instructions = ""
+# specific configurations for incremental review (/review -i)
+require_all_thresholds_for_incremental_review=false
+minimal_commits_for_incremental_review=0
+minimal_minutes_for_incremental_review=0
 
 [pr_description] # /describe #
 publish_labels=true
@@ -105,6 +109,9 @@ push_commands = [
        --pr_reviewer.num_code_suggestions=0 \
        --pr_reviewer.inline_code_comments=false \
        --pr_reviewer.remove_previous_review_comment=true \
+       --pr_reviewer.require_all_thresholds_for_incremental_review=false \
+       --pr_reviewer.minimal_commits_for_incremental_review=5 \
+       --pr_reviewer.minimal_minutes_for_incremental_review=30 \
        --pr_reviewer.extra_instructions='' \
     """
 ]

pr_agent/tools/pr_reviewer.py

@@ -1,4 +1,5 @@
 import copy
+import datetime
 from collections import OrderedDict
 from typing import List, Tuple
 
@@ -100,8 +101,7 @@ class PRReviewer:
             if self.is_auto and not get_settings().pr_reviewer.automatic_review:
                 get_logger().info(f'Automatic review is disabled {self.pr_url}')
                 return None
-            if self.is_auto and self.incremental.is_incremental and not self.incremental.first_new_commit_sha:
-                get_logger().info(f"Incremental review is enabled for {self.pr_url} but there are no new commits")
+            if self.incremental.is_incremental and not self._can_run_incremental_review():
                 return None
 
             get_logger().info(f'Reviewing PR: {self.pr_url} ...')
@@ -334,3 +334,34 @@ class PRReviewer:
                 self.git_provider.remove_comment(comment)
         except Exception as e:
             get_logger().exception(f"Failed to remove previous review comment, error: {e}")
+
+    def _can_run_incremental_review(self) -> bool:
+        """Checks if we can run incremental review according the various configurations and previous review"""
+        # checking if running is auto mode but there are no new commits
+        if self.is_auto and not self.incremental.first_new_commit_sha:
+            get_logger().info(f"Incremental review is enabled for {self.pr_url} but there are no new commits")
+            return False
+        # checking if there are enough commits to start the review
+        num_new_commits = len(self.incremental.commits_range)
+        num_commits_threshold = get_settings().pr_reviewer.minimal_commits_for_incremental_review
+        not_enough_commits = num_new_commits < num_commits_threshold
+        # checking if the commits are not too recent to start the review
+        recent_commits_threshold = datetime.datetime.now() - datetime.timedelta(
+            minutes=get_settings().pr_reviewer.minimal_minutes_for_incremental_review
+        )
+        last_seen_commit_date = (
+            self.incremental.last_seen_commit.commit.author.date if self.incremental.last_seen_commit else None
+        )
+        all_commits_too_recent = (
+            last_seen_commit_date > recent_commits_threshold if self.incremental.last_seen_commit else False
+        )
+        # check all the thresholds or just one to start the review
+        condition = any if get_settings().pr_reviewer.require_all_thresholds_for_incremental_review else all
+        if condition((not_enough_commits, all_commits_too_recent)):
+            get_logger().info(
+                f"Incremental review is enabled for {self.pr_url} but didn't pass the threshold check to run:"
+                f"\n* Number of new commits = {num_new_commits} (threshold is {num_commits_threshold})"
+                f"\n* Last seen commit date = {last_seen_commit_date} (threshold is {recent_commits_threshold})"
+            )
+            return False
+        return True