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

func get_line_link compatible with old gitlab versions #1232

Merged
merged 1 commit into from
Sep 15, 2024

Conversation

yzongyue
Copy link
Contributor

@yzongyue yzongyue commented Sep 15, 2024

User description

Gitlab only accept url hash like #L{relevant_line_start}-{relevant_line_end} before v14.1 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65453

And in the gitlab documentation, it seems that the way to write it without the second L is more recommended. ref: https://docs.gitlab.com/17.3/ee/user/project/repository/web_editor.html#link-to-specific-lines


PR Type

Enhancement


Description

  • Updated the get_line_link method in gitlab_provider.py to generate URLs compatible with older GitLab versions (pre-v14.1).
  • Removed the second 'L' in the URL hash for line ranges, changing from #L{start}-L{end} to #L{start}-{end}.
  • This change ensures better compatibility across different GitLab versions and aligns with the recommended format in GitLab documentation.
  • No other files were modified in this PR.

Changes walkthrough 📝

Relevant files
Enhancement
gitlab_provider.py
Update GitLab URL generation for compatibility                     

pr_agent/git_providers/gitlab_provider.py

  • Modified the get_line_link method to generate GitLab URLs compatible
    with older versions
  • Removed the second 'L' in the URL hash for line ranges
  • +1/-1     

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

    @qodo-merge-pro qodo-merge-pro bot added the enhancement New feature or request label Sep 15, 2024
    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add version check for GitLab URL format compatibility

    Consider adding a version check to determine whether to use the old or new URL
    format. This will ensure compatibility with both older and newer GitLab versions.

    pr_agent/git_providers/gitlab_provider.py [550-557]

     def get_line_link(self, relevant_file: str, relevant_line_start: int, relevant_line_end: int = None) -> str:
         if relevant_line_start == -1:
             link = f"{self.gl.url}/{self.id_project}/-/blob/{self.mr.source_branch}/{relevant_file}?ref_type=heads"
         elif relevant_line_end:
    -        link = f"{self.gl.url}/{self.id_project}/-/blob/{self.mr.source_branch}/{relevant_file}?ref_type=heads#L{relevant_line_start}-{relevant_line_end}"
    +        if self.gl.version.major >= 11:
    +            link = f"{self.gl.url}/{self.id_project}/-/blob/{self.mr.source_branch}/{relevant_file}?ref_type=heads#L{relevant_line_start}-L{relevant_line_end}"
    +        else:
    +            link = f"{self.gl.url}/{self.id_project}/-/blob/{self.mr.source_branch}/{relevant_file}?ref_type=heads#L{relevant_line_start}-{relevant_line_end}"
         else:
             link = f"{self.gl.url}/{self.id_project}/-/blob/{self.mr.source_branch}/{relevant_file}?ref_type=heads#L{relevant_line_start}"
         return link
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential compatibility issue with different GitLab versions, which is important for maintaining broad support and preventing potential bugs.

    8

    @mrT23
    Copy link
    Collaborator

    mrT23 commented Sep 15, 2024

    Thanks for the PR.
    I am not sure about it.

    The new format (as of 3 years ago) is with the double-L :
    image

    I am afraid that sticking with the old format will lead to legacy issues, and that it will be discontinued at some point.

    @yzongyue
    Copy link
    Contributor Author

    Thanks for the PR. I am not sure about it.

    The new format (as of 3 years ago) is with the double-L : image

    I am afraid that sticking with the old format will lead to legacy issues, and that it will be discontinued at some point.

    Thanks for review.
    As issue said, double L is for improve GitLab repositories to recognize and accept the format for URLs from GitHub sources.
    URLs containing highlighted lines generated by lastest Gitlab version use single L.
    I think single L is not an old format, but a standard practice in Gitlab

    @mrT23
    Copy link
    Collaborator

    mrT23 commented Sep 15, 2024

    ok. thanks

    @mrT23 mrT23 merged commit 16763d8 into Codium-ai:main Sep 15, 2024
    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