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

Add repository.ParseWithHost #41

Merged
merged 1 commit into from May 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 41 additions & 4 deletions pkg/repository/repository.go
Expand Up @@ -4,9 +4,9 @@ package repository

import (
"fmt"
"os"
"strings"

"github.com/cli/go-gh/internal/config"
"github.com/cli/go-gh/internal/git"
irepo "github.com/cli/go-gh/internal/repository"
)
Expand All @@ -20,6 +20,7 @@ type Repository interface {

// Parse extracts the repository information from the following
// string formats: "OWNER/REPO", "HOST/OWNER/REPO", and a full URL.
// If the format does not specify a host, use the config to determine a host.
func Parse(s string) (Repository, error) {
if git.IsURL(s) {
u, err := git.ParseURL(s)
Expand All @@ -46,12 +47,48 @@ func Parse(s string) (Repository, error) {
case 3:
return irepo.New(parts[0], parts[1], parts[2]), nil
case 2:
host := os.Getenv("GH_HOST")
if host == "" {
host = "github.com"
host := "github.com"
cfg, err := config.Load()
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems correct for now, but ultimately I find it surprising that Parse() implicitly loads config. I think an ideal API might be one where a repository "parser" is derived from a config object, so that their relationship is denoted explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree it is a bit odd. go-gh is also targeting extension authors, I would think that they don't necessarily want to concern themselves with loading up the config and determining the correct host. We also do this when initializing the various API clients. As long as there are alternatives for advanced users, that don't want automatic resolution such as repository.ParseWithHost, I am okay with having these easy to use alternatives.

if err == nil {
host = cfg.Host()
}
return irepo.New(host, parts[0], parts[1]), nil
default:
return nil, fmt.Errorf(`expected the "[HOST/]OWNER/REPO" format, got %q`, s)
}
}

// Parse extracts the repository information from the following
// string formats: "OWNER/REPO", "HOST/OWNER/REPO", and a full URL.
// If the format does not specify a host, use the host provided.
func ParseWithHost(s, host string) (Repository, error) {
if git.IsURL(s) {
u, err := git.ParseURL(s)
if err != nil {
return nil, err
}

host, owner, name, err := git.RepoInfoFromURL(u)
if err != nil {
return nil, err
}

return irepo.New(host, owner, name), nil
}

parts := strings.SplitN(s, "/", 4)
for _, p := range parts {
if len(p) == 0 {
return nil, fmt.Errorf(`expected the "[HOST/]OWNER/REPO" format, got %q`, s)
}
}

switch len(parts) {
case 3:
return irepo.New(parts[0], parts[1], parts[2]), nil
case 2:
return irepo.New(host, parts[0], parts[1]), nil
default:
return nil, fmt.Errorf(`expected the "[HOST/]OWNER/REPO" format, got %q`, s)
}
}
105 changes: 105 additions & 0 deletions pkg/repository/repository_test.go
Expand Up @@ -2,6 +2,7 @@ package repository

import (
"os"
"path/filepath"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -96,3 +97,107 @@ func TestParse(t *testing.T) {
})
}
}

func TestParse_hostFromConfig(t *testing.T) {
tempDir := t.TempDir()
old := os.Getenv("GH_CONFIG_DIR")
os.Setenv("GH_CONFIG_DIR", tempDir)
defer os.Setenv("GH_CONFIG_DIR", old)
var configData = `
git_protocol: ssh
editor:
prompt: enabled
pager: less
`
var hostData = `
enterprise.com:
user: user2
oauth_token: yyyyyyyyyyyyyyyyyyyy
git_protocol: https
`
err := os.WriteFile(filepath.Join(tempDir, "config.yml"), []byte(configData), 0644)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(tempDir, "hosts.yml"), []byte(hostData), 0644)
assert.NoError(t, err)
r, err := Parse("OWNER/REPO")
assert.NoError(t, err)
assert.Equal(t, "enterprise.com", r.Host())
assert.Equal(t, "OWNER", r.Owner())
assert.Equal(t, "REPO", r.Name())
}

func TestParseWithHost(t *testing.T) {
tests := []struct {
name string
input string
host string
wantOwner string
wantName string
wantHost string
wantErr string
}{
{
name: "OWNER/REPO combo",
input: "OWNER/REPO",
host: "github.com",
wantHost: "github.com",
wantOwner: "OWNER",
wantName: "REPO",
},
{
name: "too few elements",
input: "OWNER",
host: "github.com",
wantErr: `expected the "[HOST/]OWNER/REPO" format, got "OWNER"`,
},
{
name: "too many elements",
input: "a/b/c/d",
host: "github.com",
wantErr: `expected the "[HOST/]OWNER/REPO" format, got "a/b/c/d"`,
},
{
name: "blank value",
input: "a/",
host: "github.com",
wantErr: `expected the "[HOST/]OWNER/REPO" format, got "a/"`,
},
{
name: "with hostname",
input: "example.org/OWNER/REPO",
host: "github.com",
wantHost: "example.org",
wantOwner: "OWNER",
wantName: "REPO",
},
{
name: "full URL",
input: "https://example.org/OWNER/REPO.git",
host: "github.com",
wantHost: "example.org",
wantOwner: "OWNER",
wantName: "REPO",
},
{
name: "SSH URL",
input: "git@example.org:OWNER/REPO.git",
host: "github.com",
wantHost: "example.org",
wantOwner: "OWNER",
wantName: "REPO",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
r, err := ParseWithHost(tt.input, tt.host)
if tt.wantErr != "" {
assert.EqualError(t, err, tt.wantErr)
return
}
assert.NoError(t, err)
assert.Equal(t, tt.wantHost, r.Host())
assert.Equal(t, tt.wantOwner, r.Owner())
assert.Equal(t, tt.wantName, r.Name())
})
}
}