Standardize repository slug retrieval on single cached implementation#3658
Standardize repository slug retrieval on single cached implementation#3658
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR successfully standardizes repository slug retrieval across the codebase by eliminating duplicate implementations and consolidating on a single cached implementation (GetCurrentRepoSlug()). The refactoring removes approximately 28 lines of duplicate code while maintaining consistent behavior and adding a new helper function for splitting repository slugs.
Key Changes:
- Removed
getCurrentRepo()frompr_command.go(22 lines) andgetCurrentRepositoryInfo()fromtrial_command.go(4 lines) - Added
SplitRepoSlug()helper function torepo.gofor consistent owner/repo splitting with validation - Updated 5 call sites across
pr_command.go,trial_command.go, andspec.goto use the standardized pattern
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/cli/repo.go | Added SplitRepoSlug() helper function to split "owner/repo" format with validation |
| pkg/cli/trial_command.go | Removed getCurrentRepositoryInfo() wrapper function and updated call site to use GetCurrentRepoSlug() directly |
| pkg/cli/spec.go | Updated two call sites to use GetCurrentRepoSlug() instead of removed wrapper function |
| pkg/cli/pr_command.go | Removed getCurrentRepo() function and refactored two call sites in transferPR() to use new standardized pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func SplitRepoSlug(slug string) (owner, repo string, err error) { | ||
| parts := strings.Split(slug, "/") | ||
| if len(parts) != 2 { | ||
| return "", "", fmt.Errorf("invalid repo format: %s", slug) | ||
| } | ||
| return parts[0], parts[1], nil | ||
| } |
There was a problem hiding this comment.
The SplitRepoSlug function does not validate that owner and repo parts are non-empty strings. The existing getCurrentRepoSlugUncached function (lines 42-43 and 84) checks both len(parts) == 2 AND parts[0] != "" && parts[1] != "". This inconsistency means SplitRepoSlug would accept invalid slugs like "/repo" or "owner/" that the rest of the codebase rejects. Add validation: if len(parts) != 2 || parts[0] == "" || parts[1] == "" { ... }
| workingDir = "." | ||
| slug, err := GetCurrentRepoSlug() | ||
| if err == nil { | ||
| currentOwner, currentRepoName, err := SplitRepoSlug(slug) |
There was a problem hiding this comment.
Variable shadowing issue: the err variable from line 600 is shadowed by the := assignment on line 602. This means if SplitRepoSlug returns an error, the outer error check on line 603 will evaluate the wrong error (from GetCurrentRepoSlug). Use var currentOwner, currentRepoName string before line 602 and then currentOwner, currentRepoName, err = SplitRepoSlug(slug) with = instead of :=.
| currentOwner, currentRepoName, err := SplitRepoSlug(slug) | |
| var currentOwner, currentRepoName string | |
| currentOwner, currentRepoName, err = SplitRepoSlug(slug) |
Eliminates duplicate repository slug retrieval implementations across CLI commands by standardizing on
GetCurrentRepoSlug().Changes
Removed duplicate implementations:
getCurrentRepo()frompr_command.go(22 lines) - JSON-based approach returning owner/repo separatelygetCurrentRepositoryInfo()fromtrial_command.go(4 lines) - wrapper functionAdded helper:
SplitRepoSlug()inrepo.go- validates and splits "owner/repo" format with error handlingUpdated call sites:
pr_command.go(2 usages),trial_command.go(1 usage),spec.go(2 usages)Pattern
All repository lookups now use the cached implementation with consistent error handling and git remote fallback.
Closes #3525
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.