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

Properly fetch changed files #902

Merged
merged 3 commits into from
May 15, 2024
Merged

Properly fetch changed files #902

merged 3 commits into from
May 15, 2024

Conversation

mrT23
Copy link
Collaborator

@mrT23 mrT23 commented May 15, 2024

User description

…ion, improving accuracy of diff file identification


PR Type

Enhancement


Description

  • Refactored Azure DevOps provider to utilize PR iterations for detecting changes, enhancing the accuracy of diff file identification.
  • Updated error logging to use formatted strings for better readability and maintenance.
  • Removed the previously used commit-based change detection logic, simplifying the codebase.

Changes walkthrough 📝

Relevant files
Enhancement
azuredevops_provider.py
Refactor Azure DevOps Provider to Use PR Iterations           

pr_agent/git_providers/azuredevops_provider.py

  • Refactored to use PR iterations instead of commits for change
    detection.
  • Improved error logging with formatted strings.
  • Removed outdated code related to commit-based change detection.
  • +52/-28 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    …ion, improving accuracy of diff file identification
    Copy link
    Contributor

    PR Description updated to latest commit (e4565f7)

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves refactoring and enhancing the change detection logic in a specific provider, which requires understanding both the previous and new logic to ensure correctness. The changes are moderate in size but high in impact, requiring careful review of the logic and testing.

    🏅 Score

    85

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The refactoring switches from using commits to PR iterations for change detection. If iterations are not handled correctly or if there are edge cases not considered (e.g., handling of empty iterations or errors in fetching iterations), this could lead to incorrect file change detections.

    Error Handling: The refactoring includes changes to error logging. It's crucial to ensure that these changes do not obscure the visibility of errors or make debugging more difficult.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Improve robustness by safely accessing dictionary keys

    Avoid using dictionary access directly on the additional_properties without checking if
    the key exists. This can lead to a KeyError if the expected keys are not present. Use the
    get method with a default value to ensure robustness.

    pr_agent/git_providers/azuredevops_provider.py [256-258]

    -c = change.additional_properties['item']
    -diffs.append(c['path'])
    -diff_types[c['path']] = change.additional_properties['changeType']
    +item = change.additional_properties.get('item', {})
    +path = item.get('path', None)
    +if path:
    +    diffs.append(path)
    +    diff_types[path] = change.additional_properties.get('changeType', 'Unknown')
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a potential KeyError from direct dictionary access and proposes a safer method using .get(). This is crucial for preventing runtime errors in dynamic data handling scenarios.

    8
    Ensure the changes object is properly checked before use

    Consider checking the type of changes before iterating over changes.change_entries. This
    ensures that the changes object is not None and has the attribute change_entries,
    preventing attribute errors.

    pr_agent/git_providers/azuredevops_provider.py [254-258]

    -if changes:
    +if changes and hasattr(changes, 'change_entries'):
         for change in changes.change_entries:
             c = change.additional_properties['item']
             diffs.append(c['path'])
             diff_types[c['path']] = change.additional_properties['changeType']
     
    Suggestion importance[1-10]: 7

    Why: This suggestion correctly addresses the need to verify the existence of an attribute before accessing it, which can prevent AttributeError. This is a good practice in dynamic attribute handling.

    7
    Maintainability
    Remove commented-out old implementation to clean up the codebase

    Remove the commented-out code related to the old implementation of getting PR commits and
    changes. Keeping old code commented out can clutter the codebase and reduce
    maintainability.

    pr_agent/git_providers/azuredevops_provider.py [260-283]

    -# wrong implementation - gets all the files that were changed in any commit in the PR
    -# commits = self.azure_devops_client.get_pull_request_commits(
    -#     project=self.workspace_slug,
    -#     repository_id=self.repo_slug,
    -#     pull_request_id=self.pr_num,
    -# )
    -# ...
    +# Code related to old implementation removed for clarity and maintainability.
     
    Suggestion importance[1-10]: 6

    Why: Removing commented-out code can indeed help in maintaining a cleaner and more readable codebase. This suggestion is relevant as it targets newly commented-out code in the PR, promoting better maintainability.

    6

    mrT23 added 2 commits May 15, 2024 09:05
    …ion, improving accuracy of diff file identification
    …ion, improving accuracy of diff file identification
    @mrT23
    Copy link
    Collaborator Author

    mrT23 commented May 15, 2024

    @SagiMedina this is a competitor for the most severe bug ever ...

    if I added and deleted a file, you would have marked it as part of the PR.
    Also the 'editType' can be wrong with previous method

    @mrT23 mrT23 changed the title Refactor Azure DevOps provider to use PR iterations for change detect… Properly fetch changed files May 15, 2024
    @mrT23 mrT23 added the Bug fix label May 15, 2024
    @mrT23 mrT23 merged commit be701aa into main May 15, 2024
    1 check passed
    @mrT23 mrT23 deleted the tr/self_reflect branch May 15, 2024 06:22
    @bobbercheng
    Copy link

    /similar_issue

    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