From 5477469a91f1d39dce1364ac1d1063c494f224ba Mon Sep 17 00:00:00 2001 From: idavidov Date: Sat, 19 Aug 2023 15:06:22 +0300 Subject: [PATCH 1/7] in order to have exact sha's we have to find correct diff for this change otherwise gitlab web doesn't able show diff on page and return 500 or 400 errors based on different scenarios --- pr_agent/git_providers/gitlab_provider.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index 73a3a2f9..94b40c92 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -155,7 +155,8 @@ class GitLabProvider(GitProvider): if not found: logging.info(f"Could not find position for {relevant_file} {relevant_line_in_file}") else: - d = self.last_diff + # in order to have exact sha's we have to find correct diff for this change + d = self.get_relevant_diff(relevant_file, relevant_line_in_file) pos_obj = {'position_type': 'text', 'new_path': target_file.filename, 'old_path': target_file.old_filename if target_file.old_filename else target_file.filename, @@ -171,6 +172,14 @@ class GitLabProvider(GitProvider): self.mr.discussions.create({'body': body, 'position': pos_obj}) + def get_relevant_diff(self, relevant_file, relevant_line_in_file): + for d in self.mr.diffs.list(get_all=True): + changes = self.mr.changes() # Retrieve the changes for the merge request + for change in changes['changes']: + if change['new_path'] == relevant_file and relevant_line_in_file in change['diff']: + return d + return self.last_diff # fallback to last_diff if no relevant diff is found + def publish_code_suggestions(self, code_suggestions: list): for suggestion in code_suggestions: try: From 7b7e91319594a5dd793e7ba80d488d0195fb6f81 Mon Sep 17 00:00:00 2001 From: idavidov Date: Sat, 19 Aug 2023 15:31:02 +0300 Subject: [PATCH 2/7] to changes suggested by /improve with my small touch --- pr_agent/git_providers/gitlab_provider.py | 8 ++++++-- pr_agent/settings/configuration.toml | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index 94b40c92..ef7210ee 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -173,11 +173,15 @@ class GitLabProvider(GitProvider): 'position': pos_obj}) def get_relevant_diff(self, relevant_file, relevant_line_in_file): - for d in self.mr.diffs.list(get_all=True): - changes = self.mr.changes() # Retrieve the changes for the merge request + changes = self.mr.changes() # Retrieve the changes for the merge request once + all_diffs = self.mr.diffs.list(get_all=True) + + for d in all_diffs: for change in changes['changes']: if change['new_path'] == relevant_file and relevant_line_in_file in change['diff']: return d + logging.debug( + f'No relevant diff found for {relevant_file} {relevant_line_in_file}. Falling back to last diff.') return self.last_diff # fallback to last_diff if no relevant diff is found def publish_code_suggestions(self, code_suggestions: list): diff --git a/pr_agent/settings/configuration.toml b/pr_agent/settings/configuration.toml index 0c502df9..777d7209 100644 --- a/pr_agent/settings/configuration.toml +++ b/pr_agent/settings/configuration.toml @@ -1,7 +1,7 @@ [config] -model="gpt-4" +model="gpt-3.5-turbo-16k" fallback_models=["gpt-3.5-turbo-16k"] -git_provider="github" +git_provider="gitlab" publish_output=true publish_output_progress=true verbosity_level=0 # 0,1,2 From fdd16f6c75aa4f88baf9183410b68a85949c01de Mon Sep 17 00:00:00 2001 From: idavidov Date: Sat, 19 Aug 2023 15:40:40 +0300 Subject: [PATCH 3/7] raize exception when no diffs in MR --- pr_agent/git_providers/gitlab_provider.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index ef7210ee..6a9f9fe2 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -174,8 +174,13 @@ class GitLabProvider(GitProvider): def get_relevant_diff(self, relevant_file, relevant_line_in_file): changes = self.mr.changes() # Retrieve the changes for the merge request once + if not changes: + logging.error('No changes found for the merge request.') + return None all_diffs = self.mr.diffs.list(get_all=True) - + if not all_diffs: + logging.error('No diffs found for the merge request.') + raise ValueError(f"Could not get diff for merge request {self.id_mr}") for d in all_diffs: for change in changes['changes']: if change['new_path'] == relevant_file and relevant_line_in_file in change['diff']: From 6595c3e0c9a23faa5f0e169363e96617624bc3b5 Mon Sep 17 00:00:00 2001 From: idavidov Date: Sat, 19 Aug 2023 15:47:45 +0300 Subject: [PATCH 4/7] 2 more /improve good suggestions --- pr_agent/git_providers/gitlab_provider.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index 6a9f9fe2..6a04f6bf 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -172,7 +172,7 @@ class GitLabProvider(GitProvider): self.mr.discussions.create({'body': body, 'position': pos_obj}) - def get_relevant_diff(self, relevant_file, relevant_line_in_file): + def get_relevant_diff(self, relevant_file: str, relevant_line_in_file: int) -> Optional[dict]: changes = self.mr.changes() # Retrieve the changes for the merge request once if not changes: logging.error('No changes found for the merge request.') @@ -181,10 +181,10 @@ class GitLabProvider(GitProvider): if not all_diffs: logging.error('No diffs found for the merge request.') raise ValueError(f"Could not get diff for merge request {self.id_mr}") - for d in all_diffs: + for diff in all_diffs: for change in changes['changes']: if change['new_path'] == relevant_file and relevant_line_in_file in change['diff']: - return d + return diff logging.debug( f'No relevant diff found for {relevant_file} {relevant_line_in_file}. Falling back to last diff.') return self.last_diff # fallback to last_diff if no relevant diff is found From 50125ae57f8c3f18e787064e28d32e02955f9e04 Mon Sep 17 00:00:00 2001 From: idavidov Date: Sat, 19 Aug 2023 16:12:48 +0300 Subject: [PATCH 5/7] various changes as outcomes from AI review --- pr_agent/git_providers/gitlab_provider.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index 6a04f6bf..82024375 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -156,11 +156,14 @@ class GitLabProvider(GitProvider): logging.info(f"Could not find position for {relevant_file} {relevant_line_in_file}") else: # in order to have exact sha's we have to find correct diff for this change - d = self.get_relevant_diff(relevant_file, relevant_line_in_file) + diff = self.get_relevant_diff(relevant_file, relevant_line_in_file) + if diff is None: + logger.error(f"Could not get diff for merge request {self.id_mr}") + raise ValueError(f"Could not get diff for merge request {self.id_mr}") pos_obj = {'position_type': 'text', 'new_path': target_file.filename, 'old_path': target_file.old_filename if target_file.old_filename else target_file.filename, - 'base_sha': d.base_commit_sha, 'start_sha': d.start_commit_sha, 'head_sha': d.head_commit_sha} + 'base_sha': diff.base_commit_sha, 'start_sha': diff.start_commit_sha, 'head_sha': diff.head_commit_sha} if edit_type == 'deletion': pos_obj['old_line'] = source_line_no - 1 elif edit_type == 'addition': @@ -180,7 +183,7 @@ class GitLabProvider(GitProvider): all_diffs = self.mr.diffs.list(get_all=True) if not all_diffs: logging.error('No diffs found for the merge request.') - raise ValueError(f"Could not get diff for merge request {self.id_mr}") + return None for diff in all_diffs: for change in changes['changes']: if change['new_path'] == relevant_file and relevant_line_in_file in change['diff']: From 35afe758e94d0a092050b04fb820885c2ccf6a4c Mon Sep 17 00:00:00 2001 From: idavidov Date: Sat, 19 Aug 2023 16:16:16 +0300 Subject: [PATCH 6/7] revert back conf --- pr_agent/settings/configuration.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pr_agent/settings/configuration.toml b/pr_agent/settings/configuration.toml index 777d7209..0c502df9 100644 --- a/pr_agent/settings/configuration.toml +++ b/pr_agent/settings/configuration.toml @@ -1,7 +1,7 @@ [config] -model="gpt-3.5-turbo-16k" +model="gpt-4" fallback_models=["gpt-3.5-turbo-16k"] -git_provider="gitlab" +git_provider="github" publish_output=true publish_output_progress=true verbosity_level=0 # 0,1,2 From 9770f4709a2b3e50638f0f5820a654a02334b799 Mon Sep 17 00:00:00 2001 From: idavidov Date: Sat, 19 Aug 2023 16:26:15 +0300 Subject: [PATCH 7/7] few more changes suggested by AI implemented --- pr_agent/git_providers/gitlab_provider.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index 82024375..ec6f236d 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -14,6 +14,9 @@ from .git_provider import EDIT_TYPE, FilePatchInfo, GitProvider logger = logging.getLogger() +class DiffNotFoundError(Exception): + """Raised when the diff for a merge request cannot be found.""" + pass class GitLabProvider(GitProvider): @@ -56,7 +59,7 @@ class GitLabProvider(GitProvider): self.last_diff = self.mr.diffs.list(get_all=True)[-1] except IndexError as e: logger.error(f"Could not get diff for merge request {self.id_mr}") - raise ValueError(f"Could not get diff for merge request {self.id_mr}") from e + raise DiffNotFoundError(f"Could not get diff for merge request {self.id_mr}") from e def _get_pr_file_content(self, file_path: str, branch: str) -> str: @@ -150,8 +153,8 @@ class GitLabProvider(GitProvider): def create_inline_comments(self, comments: list[dict]): raise NotImplementedError("Gitlab provider does not support publishing inline comments yet") - def send_inline_comment(self, body, edit_type, found, relevant_file, relevant_line_in_file, source_line_no, - target_file, target_line_no): + def send_inline_comment(self,body: str,edit_type: str,found: bool,relevant_file: str,relevant_line_in_file: int, + source_line_no: int, target_file: str,target_line_no: int) -> None: if not found: logging.info(f"Could not find position for {relevant_file} {relevant_line_in_file}") else: @@ -159,7 +162,7 @@ class GitLabProvider(GitProvider): diff = self.get_relevant_diff(relevant_file, relevant_line_in_file) if diff is None: logger.error(f"Could not get diff for merge request {self.id_mr}") - raise ValueError(f"Could not get diff for merge request {self.id_mr}") + raise DiffNotFoundError(f"Could not get diff for merge request {self.id_mr}") pos_obj = {'position_type': 'text', 'new_path': target_file.filename, 'old_path': target_file.old_filename if target_file.old_filename else target_file.filename,