-
-
Notifications
You must be signed in to change notification settings - Fork 285
Add support for GitHub pull request URLs #4295
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for installing packages directly from GitHub pull request URLs by transforming them into the appropriate git reference format and fetching the correct refspecs.
- Introduces
expand_github_urlto parse PR URLs in the REPL parser - Fetches and resolves pull request refs in
handle_repo_add!andget_object_or_branch - Adds tests for PR URL support and updates the CHANGELOG
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/new.jl | Added a test case for parsing a GitHub PR URL |
| src/Types.jl | Fetched PR refs with custom refspec and resolved them |
| src/REPLMode/argument_parsers.jl | Introduced expand_github_url to handle PR URLs |
| CHANGELOG.md | Documented PR URL support in release notes |
Comments suppressed due to low confidence (2)
src/Types.jl:909
- [nitpick] The variable name
pr_refspecssuggests multiple refspecs but contains only one entry. Consider renaming it topr_refspecfor clarity.
pr_refspecs = ["+refs/pull/*/head:refs/remotes/cache/pull/*/head"]
src/REPLMode/argument_parsers.jl:36
- [nitpick] Consider adding a docstring to
expand_github_urlto describe its behavior and the URL patterns it handles, which will improve readability and maintenance.
function expand_github_url(word::String)::Vector{String}
src/Types.jl
Outdated
| GitTools.fetch(ctx.io, repo, repo_source_typed; refspecs=refspecs) | ||
| # For pull requests, fetch the specific PR ref | ||
| if startswith(rev_or_hash, "pull/") && endswith(rev_or_hash, "/head") | ||
| pr_refspecs = ["+refs/pull/*/head:refs/remotes/cache/pull/*/head"] |
Copilot
AI
Jul 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a wildcard refspec '+refs/pull//head:refs/remotes/cache/pull//head' fetches all pull request refs. It would be more efficient to fetch only the specific PR by interpolating the PR number (e.g., '+refs/pull/529/head:refs/remotes/cache/pull/529/head').
| pr_refspecs = ["+refs/pull/*/head:refs/remotes/cache/pull/*/head"] | |
| pr_number = replace(rev_or_hash, r"^pull/|/head$" => "") | |
| pr_refspecs = ["+refs/pull/$pr_number/head:refs/remotes/cache/pull/$pr_number/head"] |
| gitobject = LibGit2.GitObject(repo, "remotes/cache/" * rev) | ||
| return gitobject, true | ||
| catch err | ||
| err isa LibGit2.GitError && err.code == LibGit2.Error.ENOTFOUND || rethrow() |
Copilot
AI
Jul 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using || rethrow() makes the control flow less explicit. Consider rewriting the catch block with an explicit if check and rethrow(err) in the else branch for better readability.
| err isa LibGit2.GitError && err.code == LibGit2.Error.ENOTFOUND || rethrow() | |
| if !(err isa LibGit2.GitError && err.code == LibGit2.Error.ENOTFOUND) | |
| rethrow(err) | |
| end |
src/REPLMode/argument_parsers.jl
Outdated
| if (m = match(r"https://github.com/(.*?)/(.*?)/(?:tree|commit)/(.*?)$", word)) !== nothing | ||
| return ["https://github.com/$(m.captures[1])/$(m.captures[2])", "#$(m.captures[3])"] | ||
| # Handle GitHub pull request URLs | ||
| elseif (m = match(r"https://github.com/(.*?)/(.*?)/pull/(\d+)$", word)) !== nothing |
Copilot
AI
Jul 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This regex only matches PR URLs without trailing slashes or '.git' suffix. Consider extending it to allow an optional trailing slash and optional '.git' extension to support more URL variants.
| elseif (m = match(r"https://github.com/(.*?)/(.*?)/pull/(\d+)$", word)) !== nothing | |
| elseif (m = match(r"https://github.com/(.*?)/(.*?)/pull/(\d+)(?:\.git)?/?$", word)) !== nothing |
Allow direct package installation from GitHub PR URLs such as: pkg> add https://github.com/Org/Package.jl/pull/123 This transforms PR URLs into the proper git reference format (pull/<number>/head) and ensures the correct refspecs are fetched to resolve the pull request. Fixes #2230 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Allow direct package installation from GitHub PR URLs such as: pkg> add https://github.com/Org/Package.jl/pull/123
This transforms PR URLs into the proper git reference format (pull//head) and ensures the correct refspecs are fetched to resolve the pull request.
Fixes #3975
Fixes #740
🤖 Generated with Claude Code