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: Logging and Commenting #1634

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ishaansehgal99
Copy link

@ishaansehgal99 ishaansehgal99 commented Mar 20, 2025

User description

Fixed identation added logs.


PR Type

Bug fix, Enhancement


Description

  • Fixed indentation issues in multiple function calls.

  • Enhanced logging messages for better debugging.

  • Added debug logs for request handling and filtering.

  • Improved error logging with detailed exception messages.


Changes walkthrough 📝

Relevant files
Bug fix
github_app.py
Fix indentation and enhance logging functionality               

pr_agent/servers/github_app.py

  • Fixed indentation in _perform_auto_commands_github calls.
  • Improved error logging with exception details in get_log_context.
  • Added debug logs for request handling and filtering logic.
  • Enhanced logging for incremental review and ignored requests.
  • +8/-4     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Verified

    This commit was created on GitHub.com and signed with GitHub’s verified signature.
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Logging Format

    The error logging format was changed from passing the exception as a separate parameter to including it in the f-string. This might affect how errors are displayed in logging systems that expect structured exception data.

    get_logger().error(f"Failed to get log context: {e}")

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Improve eligibility check readability

    The code is using a double negative condition with is not
    Eligibility.NOT_ELIGIBLE which makes the logic harder to understand. Consider
    using a positive condition like in [Eligibility.ELIGIBLE, Eligibility.UNKNOWN]
    for better readability.

    pr_agent/servers/github_app.py [198-200]

    -if get_identity_provider().verify_eligibility("github", sender_id, api_url) is not Eligibility.NOT_ELIGIBLE:
    +if get_identity_provider().verify_eligibility("github", sender_id, api_url) in [Eligibility.ELIGIBLE, Eligibility.UNKNOWN]:
         get_logger().info(f"Performing incremental review for {api_url=} because of {event=} and {action=}")
         await _perform_auto_commands_github("push_commands", agent, body, api_url, log_context)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion improves code readability by replacing the double negative condition with a more explicit positive condition. This makes the eligibility check logic clearer and easier to understand, which enhances maintainability.

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

    Sorry, something went wrong.

    @@ -233,7 +233,7 @@ def get_log_context(body, event, action, build_number):
    "request_id": uuid.uuid4().hex, "build_number": build_number, "app_name": app_name,
    "repo": repo, "git_org": git_org, "installation_id": installation_id}
    except Exception as e:
    get_logger().error("Failed to get log context", e)
    get_logger().error(f"Failed to get log context: {e}")
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    this is unsafe. if 'e' is a dictionary (and it can happen), it will crash

    ->
    get_logger().error(f"Failed to get log context", artifact={'error': e}

    @mrT23
    Copy link
    Collaborator

    mrT23 commented Mar 21, 2025

    thanks @ishaansehgal99
    make the error logging safer, and we can merge

    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.

    None yet

    2 participants