X Tutup
Skip to content

Commit 6a57dcf

Browse files
mislavmislav
committed
💅 cleanup placeholder implementation
1 parent 59b4d5c commit 6a57dcf

File tree

2 files changed

+76
-51
lines changed

2 files changed

+76
-51
lines changed

‎pkg/cmd/api/api.go‎

Lines changed: 32 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,9 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command
7272
"graphql" to access the GitHub API v4.
7373
7474
Placeholder values "{owner}", "{repo}", and "{branch}" in the endpoint argument will
75-
get replaced with values from the repository of the current directory.
75+
get replaced with values from the repository of the current directory. Note that in
76+
some shells, for example PowerShell, you may need to enclose any value that contains
77+
"{...}" in quotes to prevent the shell from applying special meaning to curly braces.
7678
7779
The default HTTP request method is "GET" normally and "POST" if any parameters
7880
were added. Override the method with %[1]s--method%[1]s.
@@ -397,41 +399,41 @@ func processResponse(resp *http.Response, opts *ApiOptions, headersOutputStream
397399
return
398400
}
399401

400-
var placeholderRE = regexp.MustCompile(`(\:(owner|repo|branch)\b|\{(owner|repo|branch)\})`)
402+
var placeholderRE = regexp.MustCompile(`(\:(owner|repo|branch)\b|\{[a-z]+\})`)
401403

402-
// fillPlaceholders populates `{owner}` and `{repo}` placeholders with values from the current repository
404+
// fillPlaceholders replaces placeholders with values from the current repository
403405
func fillPlaceholders(value string, opts *ApiOptions) (string, error) {
404-
if !placeholderRE.MatchString(value) {
405-
return value, nil
406-
}
406+
var err error
407+
return placeholderRE.ReplaceAllStringFunc(value, func(m string) string {
408+
var name string
409+
if m[0] == ':' {
410+
name = m[1:]
411+
} else {
412+
name = m[1 : len(m)-1]
413+
}
407414

408-
baseRepo, err := opts.BaseRepo()
409-
if err != nil {
410-
return value, err
411-
}
412-
413-
filled := placeholderRE.ReplaceAllStringFunc(value, func(m string) string {
414-
switch m {
415-
case ":owner", "{owner}":
416-
return baseRepo.RepoOwner()
417-
case ":repo", "{repo}":
418-
return baseRepo.RepoName()
419-
case ":branch", "{branch}":
420-
branch, e := opts.Branch()
421-
if e != nil {
415+
switch name {
416+
case "owner":
417+
if baseRepo, e := opts.BaseRepo(); e == nil {
418+
return baseRepo.RepoOwner()
419+
} else {
420+
err = e
421+
}
422+
case "repo":
423+
if baseRepo, e := opts.BaseRepo(); e == nil {
424+
return baseRepo.RepoName()
425+
} else {
426+
err = e
427+
}
428+
case "branch":
429+
if branch, e := opts.Branch(); e == nil {
430+
return branch
431+
} else {
422432
err = e
423433
}
424-
return branch
425-
default:
426-
panic(fmt.Sprintf("invalid placeholder: %q", m))
427434
}
428-
})
429-
430-
if err != nil {
431-
return value, err
432-
}
433-
434-
return filled, nil
435+
return m
436+
}), err
435437
}
436438

437439
func printHeaders(w io.Writer, headers http.Header, colorize bool) {

‎pkg/cmd/api/api_test.go‎

Lines changed: 44 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -978,7 +978,7 @@ func Test_fillPlaceholders(t *testing.T) {
978978
wantErr: false,
979979
},
980980
{
981-
name: "has substitutes",
981+
name: "has substitutes (colon)",
982982
args: args{
983983
value: "repos/:owner/:repo/releases",
984984
opts: &ApiOptions{
@@ -991,39 +991,37 @@ func Test_fillPlaceholders(t *testing.T) {
991991
wantErr: false,
992992
},
993993
{
994-
name: "has branch placeholder",
994+
name: "has branch placeholder (colon)",
995995
args: args{
996-
value: "repos/cli/cli/branches/:branch/protection/required_status_checks",
996+
value: "repos/owner/repo/branches/:branch/protection/required_status_checks",
997997
opts: &ApiOptions{
998-
BaseRepo: func() (ghrepo.Interface, error) {
999-
return ghrepo.New("cli", "cli"), nil
1000-
},
998+
BaseRepo: nil,
1001999
Branch: func() (string, error) {
10021000
return "trunk", nil
10031001
},
10041002
},
10051003
},
1006-
want: "repos/cli/cli/branches/trunk/protection/required_status_checks",
1004+
want: "repos/owner/repo/branches/trunk/protection/required_status_checks",
10071005
wantErr: false,
10081006
},
10091007
{
1010-
name: "has branch placeholder and git is in detached head",
1008+
name: "has branch placeholder and git is in detached head (colon)",
10111009
args: args{
10121010
value: "repos/:owner/:repo/branches/:branch",
10131011
opts: &ApiOptions{
10141012
BaseRepo: func() (ghrepo.Interface, error) {
1015-
return ghrepo.New("cli", "cli"), nil
1013+
return ghrepo.New("hubot", "robot-uprising"), nil
10161014
},
10171015
Branch: func() (string, error) {
10181016
return "", git.ErrNotOnAnyBranch
10191017
},
10201018
},
10211019
},
1022-
want: "repos/:owner/:repo/branches/:branch",
1020+
want: "repos/hubot/robot-uprising/branches/:branch",
10231021
wantErr: true,
10241022
},
10251023
{
1026-
name: "has substitutes (braces)",
1024+
name: "has substitutes",
10271025
args: args{
10281026
value: "repos/{owner}/{repo}/releases",
10291027
opts: &ApiOptions{
@@ -1036,39 +1034,53 @@ func Test_fillPlaceholders(t *testing.T) {
10361034
wantErr: false,
10371035
},
10381036
{
1039-
name: "has branch placeholder (braces)",
1037+
name: "has branch placeholder",
10401038
args: args{
1041-
value: "repos/cli/cli/branches/{branch}/protection/required_status_checks",
1039+
value: "repos/owner/repo/branches/{branch}/protection/required_status_checks",
10421040
opts: &ApiOptions{
1043-
BaseRepo: func() (ghrepo.Interface, error) {
1044-
return ghrepo.New("cli", "cli"), nil
1045-
},
1041+
BaseRepo: nil,
10461042
Branch: func() (string, error) {
10471043
return "trunk", nil
10481044
},
10491045
},
10501046
},
1051-
want: "repos/cli/cli/branches/trunk/protection/required_status_checks",
1047+
want: "repos/owner/repo/branches/trunk/protection/required_status_checks",
10521048
wantErr: false,
10531049
},
10541050
{
1055-
name: "has branch placeholder and git is in detached head (braces)",
1051+
name: "has branch placeholder and git is in detached head",
10561052
args: args{
10571053
value: "repos/{owner}/{repo}/branches/{branch}",
10581054
opts: &ApiOptions{
10591055
BaseRepo: func() (ghrepo.Interface, error) {
1060-
return ghrepo.New("cli", "cli"), nil
1056+
return ghrepo.New("hubot", "robot-uprising"), nil
10611057
},
10621058
Branch: func() (string, error) {
10631059
return "", git.ErrNotOnAnyBranch
10641060
},
10651061
},
10661062
},
1067-
want: "repos/{owner}/{repo}/branches/{branch}",
1063+
want: "repos/hubot/robot-uprising/branches/{branch}",
10681064
wantErr: true,
10691065
},
10701066
{
1071-
name: "no greedy substitutes",
1067+
name: "surfaces errors in earlier placeholders",
1068+
args: args{
1069+
value: "{branch}-{owner}",
1070+
opts: &ApiOptions{
1071+
BaseRepo: func() (ghrepo.Interface, error) {
1072+
return ghrepo.New("hubot", "robot-uprising"), nil
1073+
},
1074+
Branch: func() (string, error) {
1075+
return "", git.ErrNotOnAnyBranch
1076+
},
1077+
},
1078+
},
1079+
want: "{branch}-hubot",
1080+
wantErr: true,
1081+
},
1082+
{
1083+
name: "no greedy substitutes (colon)",
10721084
args: args{
10731085
value: ":ownership/:repository",
10741086
opts: &ApiOptions{
@@ -1078,6 +1090,17 @@ func Test_fillPlaceholders(t *testing.T) {
10781090
want: ":ownership/:repository",
10791091
wantErr: false,
10801092
},
1093+
{
1094+
name: "non-placeholders are left intact",
1095+
args: args{
1096+
value: "{}{ownership}/{repository}",
1097+
opts: &ApiOptions{
1098+
BaseRepo: nil,
1099+
},
1100+
},
1101+
want: "{}{ownership}/{repository}",
1102+
wantErr: false,
1103+
},
10811104
}
10821105
for _, tt := range tests {
10831106
t.Run(tt.name, func(t *testing.T) {

0 commit comments

Comments
 (0)
X Tutup