Skip to content

Standardize repository slug retrieval on single cached implementation#3658

Merged
pelikhan merged 2 commits intomainfrom
copilot/standardize-repo-slug-functions
Nov 11, 2025
Merged

Standardize repository slug retrieval on single cached implementation#3658
pelikhan merged 2 commits intomainfrom
copilot/standardize-repo-slug-functions

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Nov 11, 2025

Eliminates duplicate repository slug retrieval implementations across CLI commands by standardizing on GetCurrentRepoSlug().

Changes

Removed duplicate implementations:

  • getCurrentRepo() from pr_command.go (22 lines) - JSON-based approach returning owner/repo separately
  • getCurrentRepositoryInfo() from trial_command.go (4 lines) - wrapper function

Added helper:

  • SplitRepoSlug() in repo.go - validates and splits "owner/repo" format with error handling

Updated call sites:

  • pr_command.go (2 usages), trial_command.go (1 usage), spec.go (2 usages)

Pattern

// Before
owner, repo, err := getCurrentRepo()

// After
slug, err := GetCurrentRepoSlug()
if err != nil {
    return err
}
owner, repo, err := SplitRepoSlug(slug)

All repository lookups now use the cached implementation with consistent error handling and git remote fallback.

Closes #3525

Original prompt

This section details on the original issue you should resolve

<issue_title>[task] Standardize Repository Slug Functions</issue_title>
<issue_description>## Objective
Eliminate ~40-60 lines of duplicate repository slug retrieval code by standardizing on a single canonical implementation with caching.

Context

The analysis in #3525 identified 4 different implementations of repository slug retrieval across CLI command files, all using gh repo view but with different output formats and caching strategies.

Duplicate Functions:

  1. getCurrentRepo (pr_command.go:100) - Returns owner, repo separately
  2. GetCurrentRepoSlug (repo.go:95) - Returns "owner/repo" string with caching ✓ Keep this one
  3. getCurrentRepoSlugUncached (repo.go:31) - Returns "owner/repo" string without caching
  4. getCurrentRepositoryInfo (trial_command.go:461) - Wrapper around GetCurrentRepoSlug

Approach

Step 1: Identify Canonical Implementation

Keep GetCurrentRepoSlug() from pkg/cli/repo.go as the canonical implementation because:

  • It returns the standard "owner/repo" format
  • It has caching for performance
  • It's already well-tested
  • It's in a dedicated file for repository operations

Step 2: Remove Duplicate Implementations

Delete the following functions and their usages:

  • getCurrentRepo from pr_command.go (line 100)
  • getCurrentRepositoryInfo from trial_command.go (line 461)

Keep getCurrentRepoSlugUncached as it serves a specific purpose (no caching).

Step 3: Update Callers

Replace all calls to duplicate functions:

In pr_command.go:

// Before:
owner, repo, err := getCurrentRepo()
if err != nil {
    return err
}

// After:
import "github.com/githubnext/gh-aw/pkg/cli"

slug, err := GetCurrentRepoSlug()
if err != nil {
    return err
}
parts := strings.Split(slug, "/")
if len(parts) != 2 {
    return fmt.Errorf("invalid repo format: %s", slug)
}
owner, repo := parts[0], parts[1]

In trial_command.go:

// Before:
slug, err := getCurrentRepositoryInfo()

// After:
slug, err := GetCurrentRepoSlug()

Step 4: Add Helper for Splitting Slug

If splitting owner/repo is common, add a helper to repo.go:

// SplitRepoSlug splits "owner/repo" into owner and repo
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
}

Files to Modify

  • pkg/cli/pr_command.go - Remove getCurrentRepo, update callers
  • pkg/cli/trial_command.go - Remove getCurrentRepositoryInfo, update callers
  • pkg/cli/repo.go - Optionally add SplitRepoSlug helper

Acceptance Criteria

  • getCurrentRepo function removed from pr_command.go
  • getCurrentRepositoryInfo function removed from trial_command.go
  • All calls to removed functions updated to use GetCurrentRepoSlug()
  • Optional: SplitRepoSlug helper added if needed
  • All existing tests pass without modification
  • make test-unit passes
  • make lint passes
  • Estimated 40-60 lines of code removed

Benefits

  • Single canonical function with caching
  • Consistent behavior across all CLI commands
  • Clear API with single responsibility
  • Reduced confusion for developers

Estimated Impact

AI generated by Plan Command for #3525</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Standardize repository slug functions with caching Standardize repository slug retrieval on single cached implementation Nov 11, 2025
Copilot AI requested a review from pelikhan November 11, 2025 20:01
@pelikhan pelikhan marked this pull request as ready for review November 11, 2025 21:07
Copilot AI review requested due to automatic review settings November 11, 2025 21:07
@pelikhan pelikhan merged commit 568c883 into main Nov 11, 2025
75 checks passed
@pelikhan pelikhan deleted the copilot/standardize-repo-slug-functions branch November 11, 2025 21:09
Copy link
Copy Markdown
Contributor

Copilot AI left a 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 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() from pr_command.go (22 lines) and getCurrentRepositoryInfo() from trial_command.go (4 lines)
  • Added SplitRepoSlug() helper function to repo.go for consistent owner/repo splitting with validation
  • Updated 5 call sites across pr_command.go, trial_command.go, and spec.go to 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.

Comment on lines +109 to +115
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
}
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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] == "" { ... }

Copilot uses AI. Check for mistakes.
workingDir = "."
slug, err := GetCurrentRepoSlug()
if err == nil {
currentOwner, currentRepoName, err := SplitRepoSlug(slug)
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :=.

Suggested change
currentOwner, currentRepoName, err := SplitRepoSlug(slug)
var currentOwner, currentRepoName string
currentOwner, currentRepoName, err = SplitRepoSlug(slug)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[task] Standardize Repository Slug Functions [refactor] 🔧 Semantic Function Clustering Analysis: Refactoring Opportunities

3 participants