Skip to content

trailing slahes were not handled properly when parsing the owner and repo #156

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 19, 2025
Merged

trailing slahes were not handled properly when parsing the owner and repo #156

merged 4 commits into from
Apr 19, 2025

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Apr 18, 2025 •
edited by github-actions bot
Loading

User description

trailing slahes were not handled properly when parsing the owner and repo

PR Type

  • Bug fix
  • Tests

Description

  • Adjust repository URL parsing to handle trailing slashes.

  • Remove redundant commented code in git_utils.

  • Add test cases for SSH URL trailing slash scenario.

Changes walkthrough 📝

Relevant files
Bug fix
git_utils.py
Update get_repo_owner_and_name to handle trailing slashes.

codeflash/code_utils/git_utils.py

  • Added condition to handle empty string when trailing slash exists.
  • Adjusted index access in URL split for proper repo owner extraction.
    		• +4/-2     
    Tests
    test_git_utils.py
    Add new test for trailing slash in git utils parsing.       

    tests/test_git_utils.py

  • Introduced test for SSH URL with trailing slash.
  • Enhanced tests for get_repo_owner_and_name consistency.
    		• +6/-0     
    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    •

    @aseembits93 aseembits93 self-assigned this Apr 18, 2025
    @aseembits93 aseembits93 enabled auto-merge April 18, 2025 04:19
    @github-actions
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Edge Case

    The URL splitting logic assumes a fixed structure and does not validate the length of the resulting array. Consider adding a check to ensure that the split list has the required number of elements before indexing, to prevent potential IndexError when unexpected URL formats are encountered.

     
    if split_url[-1] == "":
    repo_owner_with_github, repo_name = split_url[-3], split_url[-2]
else:
    repo_owner_with_github, repo_name = split_url[-2], split_url[-1]

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Simplify trailing slash handling

    Simplify trailing slash handling by stripping the trailing slash from remote_url
    before splitting.

    codeflash/code_utils/git_utils.py [86-89]

    -if split_url[-1] == "":
-    repo_owner_with_github, repo_name = split_url[-3], split_url[-2]
-else:
-    repo_owner_with_github, repo_name = split_url[-2], split_url[-1]
+remote_url = remote_url.rstrip("/")
+split_url = remote_url.split("/")
+repo_owner_with_github, repo_name = split_url[-2], split_url[-1]
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion simplifies the conditional check by stripping the trailing slash before splitting, which is a clear and concise improvement, though it only provides a minor refactor in readability and style.

    		Low

    codeflash-ai bot added a commit that referenced this pull request Apr 18, 2025
    @codeflash-ai
    ⚡️ Speed up method `CodeFlashBenchmarkPlugin.pytest_collection_modify…
    dd05b32 
    …items` by 77% in PR #156 (`get_repo_owner_fix`)

To optimize the provided Python code for better performance, we can reduce redundant checks and improve the loop efficiency. Here's an optimized version that maintains the same functionality while potentially running faster.



### Summary of Changes.

1. **Reduced redundant checks:** Combined the fixturenames and marker checks into fewer lines and skipped the iteration as soon as the benchmark fixture or marker is found.
2. **Improved readability and clarity:** The logic is more straightforward, reducing complexity and the number of inspections on each loop iteration, potentially improving runtime performance.
    @codeflash-ai codeflash-ai bot mentioned this pull request Apr 18, 2025
    Closed
    @codeflash-ai
    Copy link
    Contributor

    codeflash-ai bot commented Apr 18, 2025

    ⚡️ Codeflash found optimizations for this PR

    📄 77% (0.77x) speedup for CodeFlashBenchmarkPlugin.pytest_collection_modifyitems in codeflash/benchmarking/plugin/plugin.py

    ⏱️ Runtime : 271 microseconds → 153 microseconds (best of 1341 runs)

    I created a new dependent PR with the suggested changes. Please review:

    If you approve, it will be merged into this PR (branch get_repo_owner_fix).

    misrasaurabh1
    misrasaurabh1 reviewed Apr 18, 2025
    View reviewed changes
    @aseembits93
    Copy link
    Contributor Author

    @misrasaurabh1 ready to approve

    @aseembits93 aseembits93 merged commit 52abe7b into main Apr 19, 2025
    16 checks passed
    @aseembits93 aseembits93 deleted the get_repo_owner_fix branch April 23, 2025 19:33
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @misrasaurabh1 misrasaurabh1 misrasaurabh1 approved these changes

    Assignees

    @aseembits93 aseembits93

    Labels

    Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @aseembits93 @misrasaurabh1