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 : support ai title with markers #1441

Merged

Conversation

benedict-lee
Copy link
Contributor

@benedict-lee benedict-lee commented Jan 6, 2025

PR Type

Bug fix, Enhancement


Description

  • Fixed handling of AI-generated PR titles with markers.

  • Added logic to remove 'PR Title' key from dictionary.

  • Improved conditional assignment of PR title based on settings.

  • Enhanced logging for description marker replacements.


Changes walkthrough 📝

Relevant files
Bug fix
pr_description.py
Fix and enhance AI title handling logic                                   

pr_agent/tools/pr_description.py

  • Fixed logic to handle AI-generated PR titles with markers.
  • Removed 'PR Title' key from dictionary for better processing.
  • Added conditional logic for assigning PR title based on settings.
  • Improved logging for description marker replacements.
  • +10/-1   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 92
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The code assumes self.data dictionary always contains a 'title' key. If neither 'title' exists in self.data nor in self.vars, this could raise a KeyError.

    ai_title = self.data.pop('title', self.vars["title"])

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add error handling for dictionary key removal operation to prevent potential runtime errors

    Add error handling for the case when self.data.pop() fails. If the 'title' key
    doesn't exist, the pop operation could raise a KeyError.

    pr_agent/tools/pr_description.py [476]

    -ai_title = self.data.pop('title', self.vars["title"])
    +try:
    +    ai_title = self.data.pop('title', self.vars["title"])
    +except Exception as e:
    +    get_logger().error(f"Error retrieving AI title {self.pr_id}: {e}")
    +    ai_title = self.vars["title"]
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While adding error handling is generally good practice, the code already uses dict.pop() with a default value (self.vars["title"]), which safely handles missing keys without raising KeyError. The suggestion would add unnecessary complexity.

    4
    • Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.

    @mrT23
    Copy link
    Collaborator

    mrT23 commented Jan 7, 2025

    @benedict-lee I am not sure about the logic of this PR.
    With markers, you aim to manually define, on each PR, where to generate auto output.

    But with your new logic, the title will be generated automatically all the time.
    It this in the spirit of "markers" ?

    @benedict-lee
    Copy link
    Contributor Author

    benedict-lee commented Jan 7, 2025

    @mrT23 According to the current logic, the PR title is not an item supported by markers.
    Only type, summary, and walkthrough are replaced with markers.

    For the title, the AI title is applied only when the generate_ai_title option is set to true. The previous logic did not reflect the PR title even when generate_ai_title was set to true, which has now been corrected.

    In summary, the title is an item independent of markers and is determined by the generate_ai_title option. The modification ensures that the title is applied correctly based on the generate_ai_title option.

    If I'm misunderstanding something, please let me know.

    @mrT23 mrT23 merged commit 7479ae3 into Codium-ai:main Jan 7, 2025
    2 checks passed
    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