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

Fix get_user_description #561

Merged
merged 3 commits into from
Jan 4, 2024
Merged

Conversation

zmeir
Copy link
Contributor

@zmeir zmeir commented Jan 3, 2024

Type

bug_fix


Description

  • Fixed the get_user_description method in git_provider.py to handle changes in the headers from "PR Type"/"PR Description"/etc to "Type"/"Description"/etc.
  • Introduced a new helper method _is_generated_by_pr_agent to check if the description was generated by the pr-agent.
  • Updated the extraction of the user description to match the new header format.
  • Updated the _prepare_pr_answer method in pr_description.py to change the formatting of the key_publish variable. The capitalize method was replaced with the title method to ensure each word in the key is capitalized.

PR changes walkthrough

Relevant files                                                                                                                                 
Bug fix
1 files
git_provider.py                                                                                         
    pr_agent/git_providers/git_provider.py

    The method get_user_description was updated to handle
    changes in the headers from "PR Type"/"PR Description"/etc
    to "Type"/"Description"/etc. A new helper method
    _is_generated_by_pr_agent was introduced to check if the
    description was generated by the pr-agent. The extraction of
    the user description was also updated to match the new
    header format.

+7/-3
Enhancement
1 files
pr_description.py                                                                                     
    pr_agent/tools/pr_description.py

    The method _prepare_pr_answer was updated to change the
    formatting of the key_publish variable. The capitalize
    method was replaced with the title method to ensure each
    word in the key is capitalized.

+1/-1

User description

The headers changed from "PR Type"/"PR Description"/etc to "Type"/"Description"/etc

The headers changed from "PR Type"/"PR Description"/etc to "Type"/"Description"/etc
Copy link
Contributor

PR Description updated to latest commit (560d30d)

Copy link
Contributor

PR Analysis

  • 🎯 Main theme: This PR addresses a bug fix in the get_user_description method in git_provider.py and introduces a new helper method _is_generated_by_pr_agent. It also updates the formatting of the key_publish variable in pr_description.py.
  • 📝 PR summary: The PR modifies the get_user_description method to handle changes in the headers from "PR Type"/"PR Description"/etc to "Type"/"Description"/etc. A new helper method _is_generated_by_pr_agent is introduced to check if the description was generated by the pr-agent. The PR also updates the _prepare_pr_answer method in pr_description.py to change the formatting of the key_publish variable.
  • 📌 Type of PR: Bug fix
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 2, because the PR is relatively small and the changes are straightforward, mainly involving string manipulations and condition checks.
  • 🔒 Security concerns: No

PR Feedback

💡 General suggestions: The PR seems to be well-structured and the changes are clearly explained. However, it would be beneficial to include unit tests to verify the new behavior and ensure that the changes work as expected. Also, consider adding comments to the new helper method _is_generated_by_pr_agent to explain its purpose and how it works.

🤖 Code feedback:
relevant filepr_agent/git_providers/git_provider.py
suggestion      

Consider using a more efficient way to check if the description starts with any of the possible headers. Currently, the code iterates over all possible headers for each description, which could be inefficient if there are many descriptions or headers. One possible way to improve this is by using a regex pattern that matches any of the headers. [medium]

relevant linereturn any(description.startswith(header) for header in possible_headers)

relevant filepr_agent/tools/pr_description.py
suggestion      

The title method capitalizes the first character of each word, which may not be desired for words like 'a', 'an', 'the', 'in', etc. Consider using a custom function that capitalizes only the first character of the sentence or using a library that provides a method to convert a string to title case properly. [medium]

relevant linekey_publish = key.rstrip(':').replace("_", " ").title()

✨ Usage tips:

To invoke the PR-Agent, add a comment using one of the following commands:

  • /review: Request a review of your Pull Request.
  • /describe: Update the PR title and description based on the contents of the PR.
  • /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
  • /ask <QUESTION>: Ask a question about the PR.
  • /update_changelog: Update the changelog based on the PR's contents.
  • /add_docs 💎: Generate docstring for new components introduced in the PR.
  • /generate_labels 💎: Generate labels for the PR based on the PR's contents.
  • /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component.

See the tools guide for more details.
To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

@@ -297,7 +297,7 @@ def _prepare_pr_answer(self) -> Tuple[str, str]:
value = self.file_label_dict
key_publish = "PR changes walkthrough"
else:
key_publish = key.rstrip(':').replace("_", " ").capitalize()
key_publish = key.rstrip(':').replace("_", " ").title()
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think this is good.
i do want to capitalize only the first letter

Copy link
Collaborator

Choose a reason for hiding this comment

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

("PR changes walkthrough"
instead of
"PR Changes Walkthrough")

Copy link
Contributor Author

@zmeir zmeir Jan 4, 2024

Choose a reason for hiding this comment

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

Sure I will revert it, I just thought since those are section titles it will be more fitting.

@zmeir
Copy link
Contributor Author

zmeir commented Jan 4, 2024

@mrT23 thanks for the review - I reverted the use of title() to capitalize(), and also made the logic to find the user description case-insensitive, so that if you change the caseness of the headers in the future it will still work (e.g. if you decide to change ## Description to ## description, ## PR Description or ## PR description, etc.).

@zmeir zmeir requested a review from mrT23 January 4, 2024 08:25
@mrT23 mrT23 merged commit b7af451 into Codium-ai:main Jan 4, 2024
@zmeir zmeir deleted the zmeir/fix/get_user_description branch January 4, 2024 08:46
yochail pushed a commit to yochail/pr-agent that referenced this pull request Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants