From 59b4d5cb7c0a3696843cf0b06db07efd8bb86e5f Mon Sep 17 00:00:00 2001 From: Robin Neatherway Date: Thu, 29 Apr 2021 16:31:31 +0100 Subject: [PATCH 1/2] Support standard path variable replacement syntax Add support for the following synonyms: {owner} for :owner {repo} for :repo {branch} for :branch --- pkg/cmd/api/api.go | 24 ++++++++-------- pkg/cmd/api/api_test.go | 61 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index da208b08a..e4cd8c041 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -71,7 +71,7 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command The endpoint argument should either be a path of a GitHub API v3 endpoint, or "graphql" to access the GitHub API v4. - Placeholder values ":owner", ":repo", and ":branch" in the endpoint argument will + Placeholder values "{owner}", "{repo}", and "{branch}" in the endpoint argument will get replaced with values from the repository of the current directory. The default HTTP request method is "GET" normally and "POST" if any parameters @@ -87,7 +87,7 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command - literal values "true", "false", "null", and integer numbers get converted to appropriate JSON types; - - placeholder values ":owner", ":repo", and ":branch" get populated with values + - placeholder values "{owner}", "{repo}", and "{branch}" get populated with values from the repository of the current directory; - if the value starts with "@", the rest of the value is interpreted as a filename to read the value from. Pass "-" to read from standard input. @@ -106,10 +106,10 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command `, "`"), Example: heredoc.Doc(` # list releases in the current repository - $ gh api repos/:owner/:repo/releases + $ gh api repos/{owner}/{repo}/releases # post an issue comment - $ gh api repos/:owner/:repo/issues/123/comments -f body='Hi from CLI' + $ gh api repos/{owner}/{repo}/issues/123/comments -f body='Hi from CLI' # add parameters to a GET request $ gh api -X GET search/issues -f q='repo:cli/cli is:open remote' @@ -121,14 +121,14 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command $ gh api --preview baptiste,nebula ... # print only specific fields from the response - $ gh api repos/:owner/:repo/issues --jq '.[].title' + $ gh api repos/{owner}/{repo}/issues --jq '.[].title' # use a template for the output - $ gh api repos/:owner/:repo/issues --template \ + $ gh api repos/{owner}/{repo}/issues --template \ '{{range .}}{{.title}} ({{.labels | pluck "name" | join ", " | color "yellow"}}){{"\n"}}{{end}}' # list releases with GraphQL - $ gh api graphql -F owner=':owner' -F name=':repo' -f query=' + $ gh api graphql -F owner='{owner}' -F name='{repo}' -f query=' query($name: String!, $owner: String!) { repository(owner: $owner, name: $name) { releases(last: 3) { @@ -397,9 +397,9 @@ func processResponse(resp *http.Response, opts *ApiOptions, headersOutputStream return } -var placeholderRE = regexp.MustCompile(`\:(owner|repo|branch)\b`) +var placeholderRE = regexp.MustCompile(`(\:(owner|repo|branch)\b|\{(owner|repo|branch)\})`) -// fillPlaceholders populates `:owner` and `:repo` placeholders with values from the current repository +// fillPlaceholders populates `{owner}` and `{repo}` placeholders with values from the current repository func fillPlaceholders(value string, opts *ApiOptions) (string, error) { if !placeholderRE.MatchString(value) { return value, nil @@ -412,11 +412,11 @@ func fillPlaceholders(value string, opts *ApiOptions) (string, error) { filled := placeholderRE.ReplaceAllStringFunc(value, func(m string) string { switch m { - case ":owner": + case ":owner", "{owner}": return baseRepo.RepoOwner() - case ":repo": + case ":repo", "{repo}": return baseRepo.RepoName() - case ":branch": + case ":branch", "{branch}": branch, e := opts.Branch() if e != nil { err = e diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 6bab83f09..a14f04292 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -870,7 +870,7 @@ func Test_magicFieldValue(t *testing.T) { wantErr: false, }, { - name: "placeholder", + name: "placeholder colon", args: args{ v: ":owner", opts: &ApiOptions{ @@ -883,6 +883,20 @@ func Test_magicFieldValue(t *testing.T) { want: "hubot", wantErr: false, }, + { + name: "placeholder braces", + args: args{ + v: "{owner}", + opts: &ApiOptions{ + IO: io, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("hubot", "robot-uprising"), nil + }, + }, + }, + want: "hubot", + wantErr: false, + }, { name: "file", args: args{ @@ -1008,6 +1022,51 @@ func Test_fillPlaceholders(t *testing.T) { want: "repos/:owner/:repo/branches/:branch", wantErr: true, }, + { + name: "has substitutes (braces)", + args: args{ + value: "repos/{owner}/{repo}/releases", + opts: &ApiOptions{ + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("hubot", "robot-uprising"), nil + }, + }, + }, + want: "repos/hubot/robot-uprising/releases", + wantErr: false, + }, + { + name: "has branch placeholder (braces)", + args: args{ + value: "repos/cli/cli/branches/{branch}/protection/required_status_checks", + opts: &ApiOptions{ + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("cli", "cli"), nil + }, + Branch: func() (string, error) { + return "trunk", nil + }, + }, + }, + want: "repos/cli/cli/branches/trunk/protection/required_status_checks", + wantErr: false, + }, + { + name: "has branch placeholder and git is in detached head (braces)", + args: args{ + value: "repos/{owner}/{repo}/branches/{branch}", + opts: &ApiOptions{ + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("cli", "cli"), nil + }, + Branch: func() (string, error) { + return "", git.ErrNotOnAnyBranch + }, + }, + }, + want: "repos/{owner}/{repo}/branches/{branch}", + wantErr: true, + }, { name: "no greedy substitutes", args: args{ From 6a57dcfd7dde07908676646dcea3021ad14c5a78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 30 Apr 2021 14:19:26 +0200 Subject: [PATCH 2/2] :nail_care: cleanup placeholder implementation --- pkg/cmd/api/api.go | 62 ++++++++++++++++++++------------------- pkg/cmd/api/api_test.go | 65 ++++++++++++++++++++++++++++------------- 2 files changed, 76 insertions(+), 51 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index e4cd8c041..27e26f004 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -72,7 +72,9 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command "graphql" to access the GitHub API v4. Placeholder values "{owner}", "{repo}", and "{branch}" in the endpoint argument will - get replaced with values from the repository of the current directory. + get replaced with values from the repository of the current directory. Note that in + some shells, for example PowerShell, you may need to enclose any value that contains + "{...}" in quotes to prevent the shell from applying special meaning to curly braces. The default HTTP request method is "GET" normally and "POST" if any parameters were added. Override the method with %[1]s--method%[1]s. @@ -397,41 +399,41 @@ func processResponse(resp *http.Response, opts *ApiOptions, headersOutputStream return } -var placeholderRE = regexp.MustCompile(`(\:(owner|repo|branch)\b|\{(owner|repo|branch)\})`) +var placeholderRE = regexp.MustCompile(`(\:(owner|repo|branch)\b|\{[a-z]+\})`) -// fillPlaceholders populates `{owner}` and `{repo}` placeholders with values from the current repository +// fillPlaceholders replaces placeholders with values from the current repository func fillPlaceholders(value string, opts *ApiOptions) (string, error) { - if !placeholderRE.MatchString(value) { - return value, nil - } + var err error + return placeholderRE.ReplaceAllStringFunc(value, func(m string) string { + var name string + if m[0] == ':' { + name = m[1:] + } else { + name = m[1 : len(m)-1] + } - baseRepo, err := opts.BaseRepo() - if err != nil { - return value, err - } - - filled := placeholderRE.ReplaceAllStringFunc(value, func(m string) string { - switch m { - case ":owner", "{owner}": - return baseRepo.RepoOwner() - case ":repo", "{repo}": - return baseRepo.RepoName() - case ":branch", "{branch}": - branch, e := opts.Branch() - if e != nil { + switch name { + case "owner": + if baseRepo, e := opts.BaseRepo(); e == nil { + return baseRepo.RepoOwner() + } else { + err = e + } + case "repo": + if baseRepo, e := opts.BaseRepo(); e == nil { + return baseRepo.RepoName() + } else { + err = e + } + case "branch": + if branch, e := opts.Branch(); e == nil { + return branch + } else { err = e } - return branch - default: - panic(fmt.Sprintf("invalid placeholder: %q", m)) } - }) - - if err != nil { - return value, err - } - - return filled, nil + return m + }), err } func printHeaders(w io.Writer, headers http.Header, colorize bool) { diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index a14f04292..acaff19da 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -978,7 +978,7 @@ func Test_fillPlaceholders(t *testing.T) { wantErr: false, }, { - name: "has substitutes", + name: "has substitutes (colon)", args: args{ value: "repos/:owner/:repo/releases", opts: &ApiOptions{ @@ -991,39 +991,37 @@ func Test_fillPlaceholders(t *testing.T) { wantErr: false, }, { - name: "has branch placeholder", + name: "has branch placeholder (colon)", args: args{ - value: "repos/cli/cli/branches/:branch/protection/required_status_checks", + value: "repos/owner/repo/branches/:branch/protection/required_status_checks", opts: &ApiOptions{ - BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.New("cli", "cli"), nil - }, + BaseRepo: nil, Branch: func() (string, error) { return "trunk", nil }, }, }, - want: "repos/cli/cli/branches/trunk/protection/required_status_checks", + want: "repos/owner/repo/branches/trunk/protection/required_status_checks", wantErr: false, }, { - name: "has branch placeholder and git is in detached head", + name: "has branch placeholder and git is in detached head (colon)", args: args{ value: "repos/:owner/:repo/branches/:branch", opts: &ApiOptions{ BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.New("cli", "cli"), nil + return ghrepo.New("hubot", "robot-uprising"), nil }, Branch: func() (string, error) { return "", git.ErrNotOnAnyBranch }, }, }, - want: "repos/:owner/:repo/branches/:branch", + want: "repos/hubot/robot-uprising/branches/:branch", wantErr: true, }, { - name: "has substitutes (braces)", + name: "has substitutes", args: args{ value: "repos/{owner}/{repo}/releases", opts: &ApiOptions{ @@ -1036,39 +1034,53 @@ func Test_fillPlaceholders(t *testing.T) { wantErr: false, }, { - name: "has branch placeholder (braces)", + name: "has branch placeholder", args: args{ - value: "repos/cli/cli/branches/{branch}/protection/required_status_checks", + value: "repos/owner/repo/branches/{branch}/protection/required_status_checks", opts: &ApiOptions{ - BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.New("cli", "cli"), nil - }, + BaseRepo: nil, Branch: func() (string, error) { return "trunk", nil }, }, }, - want: "repos/cli/cli/branches/trunk/protection/required_status_checks", + want: "repos/owner/repo/branches/trunk/protection/required_status_checks", wantErr: false, }, { - name: "has branch placeholder and git is in detached head (braces)", + name: "has branch placeholder and git is in detached head", args: args{ value: "repos/{owner}/{repo}/branches/{branch}", opts: &ApiOptions{ BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.New("cli", "cli"), nil + return ghrepo.New("hubot", "robot-uprising"), nil }, Branch: func() (string, error) { return "", git.ErrNotOnAnyBranch }, }, }, - want: "repos/{owner}/{repo}/branches/{branch}", + want: "repos/hubot/robot-uprising/branches/{branch}", wantErr: true, }, { - name: "no greedy substitutes", + name: "surfaces errors in earlier placeholders", + args: args{ + value: "{branch}-{owner}", + opts: &ApiOptions{ + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("hubot", "robot-uprising"), nil + }, + Branch: func() (string, error) { + return "", git.ErrNotOnAnyBranch + }, + }, + }, + want: "{branch}-hubot", + wantErr: true, + }, + { + name: "no greedy substitutes (colon)", args: args{ value: ":ownership/:repository", opts: &ApiOptions{ @@ -1078,6 +1090,17 @@ func Test_fillPlaceholders(t *testing.T) { want: ":ownership/:repository", wantErr: false, }, + { + name: "non-placeholders are left intact", + args: args{ + value: "{}{ownership}/{repository}", + opts: &ApiOptions{ + BaseRepo: nil, + }, + }, + want: "{}{ownership}/{repository}", + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {