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

Improve logging and error handling in Azure DevOps provider for code … #1373

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

mrT23
Copy link
Collaborator

@mrT23 mrT23 commented Nov 21, 2024

User description

…suggestions


PR Type

enhancement, error handling


Description

  • Improved logging by changing exception logs to warnings for non-critical issues.
  • Added a check to return False if there are no post parameters to process.
  • Moved the try-except block inside the loop to handle errors on a per-suggestion basis, allowing the process to continue for other suggestions.
  • Simplified the return logic to ensure the function returns True only after attempting all suggestions.

Changes walkthrough 📝

Relevant files
Error handling
azuredevops_provider.py
Improve logging and error handling in Azure DevOps provider

pr_agent/git_providers/azuredevops_provider.py

  • Changed logging from exception to warning for specific conditions.
  • Added a check for empty post parameters list before proceeding.
  • Moved try-except block inside the loop to handle individual failures.
  • Simplified return logic for the function.
  • +14/-18 

    💡 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 function now returns True even if some suggestions fail to publish. Consider if this is the desired behavior or if it should return False if any suggestion fails.

    Code Smell
    Multiple empty lines at the end of the publish_code_suggestions function could be cleaned up for better code style.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Return value does not accurately reflect the operation's success status

    The function always returns True after the loop, even if some suggestions fail to
    publish. Consider returning False if any suggestion fails to publish, to accurately
    reflect the operation's success status.

    pr_agent/git_providers/azuredevops_provider.py [114-122]

    +success = True
     try:
         self.azure_devops_client.create_thread(
             comment_thread=thread,
             project=self.workspace_slug,
             repository_id=self.repo_slug,
             pull_request_id=self.pr_num
         )
     except Exception as e:
         get_logger().warning(f"Azure failed to publish code suggestion, error: {e}")
    -return True
    +    success = False
    +return success
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a significant logical flaw where the function always returns True even when suggestions fail to publish, which could mask errors and lead to incorrect status reporting. The proposed fix using a success flag properly tracks failures.

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

    💡 Need additional feedback ? start a PR chat

    @mrT23 mrT23 merged commit 6240de3 into main Nov 21, 2024
    2 checks passed
    @mrT23 mrT23 deleted the tr/ado branch November 21, 2024 11:41
    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