Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing Thresholds for Incremental PR Reviews #423

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions pr_agent/git_providers/git_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 2 additions & 2 deletions pr_agent/git_providers/github_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,10 @@ def get_commit_range(self):
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 []

Expand Down
6 changes: 6 additions & 0 deletions pr_agent/settings/configuration.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ inline_code_comments = false
ask_and_reflect=false
automatic_review=true
remove_previous_review_comment=false
require_all_thresholds_for_incremental_review=false
Copy link
Collaborator

@mrT23 mrT23 Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you stack all the 'incremental' parameters ?
i don't want the config to be just a long incoherent list

something like

/# incremental parameters:
minimal_commits_for_incremental_review=0
minimal_minutes_for_incremental_review=0
...

minimal_commits_for_incremental_review=0
minimal_minutes_for_incremental_review=0
extra_instructions = ""

[pr_description] # /describe #
Expand Down Expand Up @@ -105,6 +108,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='' \
"""
]
Expand Down
31 changes: 28 additions & 3 deletions pr_agent/tools/pr_reviewer.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import copy
import datetime
from collections import OrderedDict
from typing import List, Tuple

Expand Down Expand Up @@ -100,9 +101,33 @@ async def run(self) -> None:
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")
return None
if self.incremental.is_incremental:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zmeir please move all of this to a dedicated, self-contained function.

this is too much content to appear directly

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 None
# 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 None

get_logger().info(f'Reviewing PR: {self.pr_url} ...')

Expand Down
Loading