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

Implement BitbucketServerProvider.get_line_link #1061

Merged
merged 1 commit into from
Jul 27, 2024

Conversation

MarkRx
Copy link
Contributor

@MarkRx MarkRx commented Jul 26, 2024

User description

Implements missing BitbucketServerProvider.get_line_link so links in PR review comments generate proper URLs.

Works with #1059

#1058


PR Type

enhancement


Description

  • Implemented the get_line_link method in BitbucketServerProvider to generate proper URLs for line links in PR review comments.
  • The method constructs URLs based on the file and line number, with special handling for cases where the line number is -1.

Changes walkthrough 📝

Relevant files
Enhancement
bitbucket_server_provider.py
Implement `get_line_link` method for BitbucketServerProvider

pr_agent/git_providers/bitbucket_server_provider.py

  • Added get_line_link method to generate proper URLs for line links.
  • Constructed URLs based on file and line number.
  • Included error handling for cases where line number is -1.
  • +7/-0     

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

    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added the enhancement New feature or request label Jul 26, 2024
    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 90
    🧪 No relevant tests
    🔒 No security concerns identified
    🔀 No multiple PR themes
    ⚡ Key issues to review

    URL Construction
    The method get_line_link constructs URLs but does not handle the scenario where relevant_line_end is provided. It currently ignores the end line even if it's passed, which might be required for range-specific links.

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for potential exceptions during URL construction

    The method get_line_link does not handle exceptions that might occur during URL
    construction, such as from quote_plus. It's advisable to add error handling to
    manage such potential issues gracefully.

    pr_agent/git_providers/bitbucket_server_provider.py [254-256]

    -link = f"{self.pr_url}/diff#{quote_plus(relevant_file)}"
    -link = f"{self.pr_url}/diff#{quote_plus(relevant_file)}?t={relevant_line_start}"
    +try:
    +    link = f"{self.pr_url}/diff#{quote_plus(relevant_file)}"
    +    if relevant_line_start != -1:
    +        link += f"?t={relevant_line_start}"
    +except Exception as e:
    +    get_logger().error(f"Error constructing URL for file {relevant_file}: {e}")
    +    raise
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Adding error handling is crucial for managing potential issues gracefully, ensuring that the application can log and handle errors without crashing.

    10
    Enhancement
    Include the relevant_line_end parameter in the link if it is provided

    The method get_line_link does not handle the optional parameter relevant_line_end.
    If relevant_line_end is provided, it should be included in the generated link to
    specify the end of the range. Modify the code to append this parameter when it is
    not None.

    pr_agent/git_providers/bitbucket_server_provider.py [253-257]

     if relevant_line_start == -1:
         link = f"{self.pr_url}/diff#{quote_plus(relevant_file)}"
     else:
    -    link = f"{self.pr_url}/diff#{quote_plus(relevant_file)}?t={relevant_line_start}"
    +    if relevant_line_end:
    +        link = f"{self.pr_url}/diff#{quote_plus(relevant_file)}?t={relevant_line_start}-{relevant_line_end}"
    +    else:
    +        link = f"{self.pr_url}/diff#{quote_plus(relevant_file)}?t={relevant_line_start}"
     return link
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion correctly enhances the functionality by including the relevant_line_end parameter, making the link more informative and useful when a range of lines is relevant.

    9
    Security
    Use urlencode for safer and more accurate URL parameter handling

    The method get_line_link currently constructs URLs by directly appending query
    parameters, which might lead to URL manipulation issues. Use a more robust method to
    construct query parameters, such as using urllib.parse.urlencode to ensure proper
    encoding and concatenation.

    pr_agent/git_providers/bitbucket_server_provider.py [254-256]

    -link = f"{self.pr_url}/diff#{quote_plus(relevant_file)}"
    -link = f"{self.pr_url}/diff#{quote_plus(relevant_file)}?t={relevant_line_start}"
    +from urllib.parse import urlencode
    +base_url = f"{self.pr_url}/diff#{quote_plus(relevant_file)}"
    +if relevant_line_start != -1:
    +    params = {'t': relevant_line_start}
    +    link = f"{base_url}?{urlencode(params)}"
    +else:
    +    link = base_url
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using urlencode ensures that query parameters are properly encoded, which enhances security and robustness of the URL construction.

    8
    Best practice
    Use None to represent an unspecified relevant_line_start instead of -1

    The condition if relevant_line_start == -1 is used to check for an invalid line
    number, but it's not clear why -1 is considered invalid. It would be better to
    document this choice or use a more standard way of indicating invalid or default
    parameters, such as None.

    pr_agent/git_providers/bitbucket_server_provider.py [253-254]

    -if relevant_line_start == -1:
    +if relevant_line_start is None:
         link = f"{self.pr_url}/diff#{quote_plus(relevant_file)}"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using None is a more standard and clear way to indicate an unspecified parameter, improving code readability and maintainability.

    7

    @mrT23 mrT23 merged commit 3a77652 into Codium-ai:main Jul 27, 2024
    1 check passed
    @mrT23
    Copy link
    Collaborator

    mrT23 commented Jul 27, 2024

    cc
    @taisyalobanov

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants