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

Conversation

mrT23
Copy link
Collaborator

@mrT23 mrT23 commented Sep 15, 2023

PR Type:

Enhancement


PR Description:

This PR introduces two main enhancements. The first one is the addition of an "Estimated effort to review" feature, which estimates the time and effort required to review a PR on a scale of 1-5. The second enhancement is the handling of cases where no language is detected in a PR. In such cases, all files are categorized under "Other". Additionally, the PR includes minor adjustments to the markdown conversion and test fixes.


PR Main Files Walkthrough:

files:

pr_agent/algo/language_handler.py: Added a condition to handle cases where no languages are detected. In such cases, all files are categorized under "Other".
pr_agent/algo/utils.py: Added a new markdown icon for the "Estimated effort to review" feature.
pr_agent/git_providers/git_provider.py: Added a condition to handle cases where no languages are detected, logging an info message in such cases.
pr_agent/tools/pr_description.py: Added support for GitHub Flavored Markdown (GFM) to the PR answer preparation. If GFM is supported, file details are wrapped in a collapsible details tag.
pr_agent/tools/pr_reviewer.py: Added a new requirement for estimating the effort to review a PR.
tests/unittest/test_language_handler.py: Updated the expected output of the test for the case where no languages are detected to include the actual files.
pr_agent/settings/configuration.toml: Added a new configuration option to require an estimate of the effort to review a PR.
pr_agent/settings/pr_reviewer_prompts.toml: Added a new prompt for estimating the effort to review a PR on a scale of 1-5.

@github-actions github-actions bot changed the title Estimated time to review Adding Estimated Time to Review Feature Sep 15, 2023
@github-actions github-actions bot added the enhancement New feature or request label Sep 15, 2023
@mrT23 mrT23 changed the title Adding Estimated Time to Review Feature Adding Estimated Review Effort Feature and Handling No Language Detection Sep 17, 2023
@Codium-ai Codium-ai deleted a comment from github-actions bot Sep 17, 2023
@mrT23 mrT23 changed the title Adding Estimated Review Effort Feature and Handling No Language Detection Adding Estimated Review Effort Feature and Handling Cases with No Detected Language Sep 17, 2023
@mrT23
Copy link
Collaborator Author

mrT23 commented Sep 17, 2023

PR Analysis

  • 🎯 Main theme: The main theme of this PR is adding a feature to estimate the review effort and handling cases with no detected language in a PR.
  • 📝 PR summary: This PR introduces two enhancements: the addition of an "Estimated effort to review" feature and handling of cases where no language is detected in a PR. It also includes minor adjustments to the markdown conversion and test fixes.
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: Yes
  • ⏱️ Estimated effort to review [1-5]: 3
    The PR introduces new features and modifies multiple files, but the changes are straightforward and well explained in the PR description.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR is well-structured and the changes are well-documented. The addition of the "Estimated effort to review" feature is a good enhancement. The handling of cases where no language is detected is also a good improvement.

  • 🤖 Code feedback:

    • relevant file: pr_agent/algo/language_handler.py
      suggestion: Consider adding a log message when no languages are detected, similar to the one in git_provider.py. This would provide more visibility into the system's behavior. [medium]
      relevant line: if not languages:

    • relevant file: pr_agent/tools/pr_reviewer.py
      suggestion: It would be beneficial to add a default value for the "require_estimate_effort_to_review" setting in the case it's not provided in the configuration. This would prevent potential errors. [medium]
      relevant line: "require_estimate_effort_to_review": get_settings().pr_reviewer.require_estimate_effort_to_review,

    • relevant file: pr_agent/tools/pr_description.py
      suggestion: It would be better to use a constant for the "gfm_markdown" string to avoid potential typos and improve maintainability. [medium]
      relevant line: if self.git_provider.is_supported("gfm_markdown"):

    • relevant file: tests/unittest/test_language_handler.py
      suggestion: It would be beneficial to add more test cases for the new feature "Estimated effort to review" to ensure its correct functionality. [important]
      relevant line: expected_output = [{'language': 'Other', 'files': files}]

@mrT23 mrT23 merged commit 1f62520 into main Sep 17, 2023
2 checks passed
@mrT23 mrT23 deleted the tr/etr branch September 17, 2023 14:10
@mrT23
Copy link
Collaborator Author

mrT23 commented Sep 20, 2023

/improve --extended

Comment on lines +234 to +241
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"
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"

Comment on lines 59 to +62
"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,
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']}

Comment on lines +89 to +95
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 %}
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).

@@ -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)": "⏱️",

@@ -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)}]

yochail pushed a commit to yochail/pr-agent that referenced this pull request Feb 11, 2024
Adding Estimated Review Effort Feature and Handling Cases with No Detected Language
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant