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

Adding Estimated Review Effort Feature and Handling Cases with No Detected Language #306

Merged
merged 6 commits into from
Sep 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
5 changes: 5 additions & 0 deletions pr_agent/algo/language_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ def sort_files_by_main_languages(languages: Dict, files: list):
files_sorted = []
rest_files = {}

# if no languages detected, put all files in the "Other" category
if not languages:
files_sorted = [({"language": "Other", "files": list(files_filtered)})]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Consider using a more Pythonic way to create a list with a single dictionary.

Suggested change
files_sorted = [({"language": "Other", "files": list(files_filtered)})]
files_sorted = [{"language": "Other", "files": list(files_filtered)}]

return files_sorted

main_extensions_flat = []
for ext in main_extensions:
main_extensions_flat.extend(ext)
Expand Down
1 change: 1 addition & 0 deletions pr_agent/algo/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ def convert_to_markdown(output_data: dict, gfm_supported: bool=True) -> str:
"General suggestions": "💡",
"Insights from user's answers": "📝",
"Code feedback": "🤖",
"Estimated effort to review [1-5]": "⏱️",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Consider using a more descriptive key for the "Estimated effort to review [1-5]" feature.

Suggested change
"Estimated effort to review [1-5]": "⏱️",
"Estimated Review Effort (1-5)": "⏱️",

}

for key, value in output_data.items():
Expand Down
4 changes: 4 additions & 0 deletions pr_agent/git_providers/git_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,10 @@ def get_main_pr_language(languages, files) -> str:
Get the main language of the commit. Return an empty string if cannot determine.
"""
main_language_str = ""
if not languages:
logging.info("No languages detected")
return main_language_str

try:
top_language = max(languages, key=languages.get).lower()

Expand Down
1 change: 1 addition & 0 deletions pr_agent/settings/configuration.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ require_focused_review=false
require_score_review=false
require_tests_review=true
require_security_review=true
require_estimate_effort_to_review=true
num_code_suggestions=4
inline_code_comments = false
ask_and_reflect=false
Expand Down
8 changes: 8 additions & 0 deletions pr_agent/settings/pr_reviewer_prompts.toml
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,14 @@ PR Analysis:
code diff changes are too scattered, then the PR is not focused. Explain
your answer shortly.
{%- endif %}
{%- if require_estimate_effort_to_review %}
Estimated effort to review [1-5]:
type: string
description: >-
Estimate, on a scale of 1-5 (inclusive), the time and effort required to review this PR by an experienced and knowledgeable developer. 1 means short and easy review , 5 means long and hard review.
Take into account the size, complexity, quality, and the needed changes of the PR code diff.
Explain your answer shortly (1-2 sentences).
{%- endif %}
Comment on lines +89 to +95
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Consider using a numeric type for the "Estimated effort to review [1-5]" input instead of a string.

Suggested change
Estimated effort to review [1-5]:
type: string
description: >-
Estimate, on a scale of 1-5 (inclusive), the time and effort required to review this PR by an experienced and knowledgeable developer. 1 means short and easy review , 5 means long and hard review.
Take into account the size, complexity, quality, and the needed changes of the PR code diff.
Explain your answer shortly (1-2 sentences).
{%- endif %}
Estimated effort to review [1-5]:
type: number
description: >-
Estimate, on a scale of 1-5 (inclusive), the time and effort required to review this PR by an experienced and knowledgeable developer. 1 means short and easy review , 5 means long and hard review.
Take into account the size, complexity, quality, and the needed changes of the PR code diff.
Explain your answer shortly (1-2 sentences).

PR Feedback:
General suggestions:
type: string
Expand Down
4 changes: 4 additions & 0 deletions pr_agent/tools/pr_description.py
Original file line number Diff line number Diff line change
Expand Up @@ -231,10 +231,14 @@ def _prepare_pr_answer(self) -> Tuple[str, str]:
pr_body += f"## {key}:\n"
if 'walkthrough' in key.lower():
# for filename, description in value.items():
if self.git_provider.is_supported("gfm_markdown"):
pr_body += "<details> <summary>files:</summary>\n\n"
for file in value:
filename = file['filename'].replace("'", "`")
description = file['changes in file']
pr_body += f'`{filename}`: {description}\n'
if self.git_provider.is_supported("gfm_markdown"):
pr_body +="</details>\n"
Comment on lines +234 to +241
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Consider using a context manager to handle the opening and closing of the details tag.

Suggested change
if self.git_provider.is_supported("gfm_markdown"):
pr_body += "<details> <summary>files:</summary>\n\n"
for file in value:
filename = file['filename'].replace("'", "`")
description = file['changes in file']
pr_body += f'`{filename}`: {description}\n'
if self.git_provider.is_supported("gfm_markdown"):
pr_body +="</details>\n"
if self.git_provider.is_supported("gfm_markdown"):
pr_body += "<details> <summary>files:</summary>\n\n"
for file in value:
filename = file['filename'].replace("'", "`")
description = file['changes in file']
pr_body += f'`{filename}`: {description}\n'
pr_body +="</details>\n"

else:
# if the value is a list, join its items by comma
if type(value) == list:
Expand Down
1 change: 1 addition & 0 deletions pr_agent/tools/pr_reviewer.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ def __init__(self, pr_url: str, is_answer: bool = False, is_auto: bool = False,
"require_tests": get_settings().pr_reviewer.require_tests_review,
"require_security": get_settings().pr_reviewer.require_security_review,
"require_focused": get_settings().pr_reviewer.require_focused_review,
"require_estimate_effort_to_review": get_settings().pr_reviewer.require_estimate_effort_to_review,
Comment on lines 59 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Consider using a dictionary comprehension to make the code more readable and concise.

Suggested change
"require_tests": get_settings().pr_reviewer.require_tests_review,
"require_security": get_settings().pr_reviewer.require_security_review,
"require_focused": get_settings().pr_reviewer.require_focused_review,
"require_estimate_effort_to_review": get_settings().pr_reviewer.require_estimate_effort_to_review,
requirements = {req: getattr(get_settings().pr_reviewer, req) for req in ['require_tests', 'require_security', 'require_focused', 'require_estimate_effort_to_review']}

'num_code_suggestions': get_settings().pr_reviewer.num_code_suggestions,
'question_str': question_str,
'answer_str': answer_str,
Expand Down
2 changes: 1 addition & 1 deletion tests/unittest/test_language_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def test_edge_case_empty_languages(self):
type('', (object,), {'filename': 'file1.py'})(),
type('', (object,), {'filename': 'file2.java'})()
]
expected_output = [{'language': 'Other', 'files': []}]
expected_output = [{'language': 'Other', 'files': files}]
assert sort_files_by_main_languages(languages, files) == expected_output

# Tests that function handles empty files list
Expand Down
Loading