STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Issue close early arg parsing

Browse source
This commit is contained in:
William Martin 2025-04-16 15:40:58 +02:00
parent aaddcb019d
commit 8b615ec2e6
4 changed files with 187 additions and 48 deletions
Download patch file Download diff file Expand all files Collapse all files

129
pkg/cmd/issue/argparsetest/argparsetest.go Normal file
View file

@ -0,0 +1,129 @@
package argparsetest
import (
"bytes"
"reflect"
"testing"
"github.com/cli/cli/v2/internal/ghrepo"
"github.com/cli/cli/v2/pkg/cmdutil"
"github.com/google/shlex"
"github.com/spf13/cobra"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// newCmdFunc represents the typical function signature we use for creating commands e.g. `NewCmdView`.
//
// It is generic over `T` as each command construction has their own Options type e.g. `ViewOptions`
type newCmdFunc[T any] func(f *cmdutil.Factory, runF func(*T) error) *cobra.Command
func TestArgParsing[T any](t *testing.T, fn newCmdFunc[T]) {
tests := []struct {
name string
input string
expectedissueNumber int
expectedBaseRepo ghrepo.Interface
expectErr bool
}{
{
name: "no argument",
input: "",
expectErr: true,
},
{
name: "issue number argument",
input: "23 --repo owner/repo",
expectedissueNumber: 23,
expectedBaseRepo: ghrepo.New("owner", "repo"),
},
{
name: "argument is hash prefixed number",
// Escaping is required here to avoid what I think is shellex treating it as a comment.
input: "\\#23 --repo owner/repo",
expectedissueNumber: 23,
expectedBaseRepo: ghrepo.New("owner", "repo"),
},
{
name: "argument is a URL",
input: "https://github.com/cli/cli/issues/23",
expectedissueNumber: 23,
expectedBaseRepo: ghrepo.New("cli", "cli"),
},
{
name: "argument cannot be parsed to an issue",
input: "unparseable",
expectErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
f := &cmdutil.Factory{}
argv, err := shlex.Split(tt.input)
assert.NoError(t, err)
var gotOpts T
cmd := fn(f, func(opts *T) error {
gotOpts = *opts
return nil
})
cmdutil.EnableRepoOverride(cmd, f)
// TODO: remember why we do this
cmd.Flags().BoolP("help", "x", false, "")
cmd.SetArgs(argv)
cmd.SetIn(&bytes.Buffer{})
cmd.SetOut(&bytes.Buffer{})
cmd.SetErr(&bytes.Buffer{})
_, err = cmd.ExecuteC()
if tt.expectErr {
require.Error(t, err)
return
} else {
require.NoError(t, err)
}
actualIssueNumber := issueNumberFromOpts(t, gotOpts)
assert.Equal(t, tt.expectedissueNumber, actualIssueNumber)
actualBaseRepo := baseRepoFromOpts(t, gotOpts)
assert.True(
t,
ghrepo.IsSame(tt.expectedBaseRepo, actualBaseRepo),
"expected base repo %+v, got %+v", tt.expectedBaseRepo, actualBaseRepo,
)
})
}
}
func issueNumberFromOpts(t *testing.T, v any) int {
rv := reflect.ValueOf(v)
field := rv.FieldByName("IssueNumber")
if !field.IsValid() || field.Kind() != reflect.Int {
t.Fatalf("Type %T does not have IssueNumber int field", v)
}
return int(field.Int())
}
func baseRepoFromOpts(t *testing.T, v any) ghrepo.Interface {
rv := reflect.ValueOf(v)
field := rv.FieldByName("BaseRepo")
// check whether the field is valid and of type func() (ghrepo.Interface, error)
if !field.IsValid() || field.Kind() != reflect.Func {
t.Fatalf("Type %T does not have BaseRepo func field", v)
}
// call the function and check the return value
results := field.Call([]reflect.Value{})
if len(results) != 2 {
t.Fatalf("%T.BaseRepo() does not return two values", v)
}
if !results[1].IsNil() {
t.Fatalf("%T.BaseRepo() returned an error: %v", v, results[1].Interface())
}
return results[0].Interface().(ghrepo.Interface)
}

29
pkg/cmd/issue/close/close.go
View file

@ -21,7 +21,7 @@ type CloseOptions struct {
IO *iostreams.IOStreams
BaseRepo func() (ghrepo.Interface, error)
SelectorArg string
IssueNumber int
Comment string
Reason string
@ -39,13 +39,23 @@ func NewCmdClose(f *cmdutil.Factory, runF func(*CloseOptions) error) *cobra.Comm
Short: "Close issue",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
// support `-R, --repo` override
opts.BaseRepo = f.BaseRepo
if len(args) > 0 {
opts.SelectorArg = args[0]
issueNumber, baseRepo, err := shared.ParseIssueFromArg(args[0])
if err != nil {
return err
}
// If the args provided the base repo then use that directly.
if baseRepo, present := baseRepo.Value(); present {
opts.BaseRepo = func() (ghrepo.Interface, error) {
return baseRepo, nil
}
} else {
// support `-R, --repo` override
opts.BaseRepo = f.BaseRepo
}
opts.IssueNumber = issueNumber
if runF != nil {
return runF(opts)
}
@ -67,7 +77,12 @@ func closeRun(opts *CloseOptions) error {
return err
}
issue, baseRepo, err := shared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.SelectorArg, []string{"id", "number", "title", "state"})
baseRepo, err := opts.BaseRepo()
if err != nil {
return err
}
issue, err := shared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, []string{"id", "number", "title", "state"})
if err != nil {
return err
}

67
pkg/cmd/issue/close/close_test.go
View file

@ -7,46 +7,32 @@ import (
fd "github.com/cli/cli/v2/internal/featuredetection"
"github.com/cli/cli/v2/internal/ghrepo"
"github.com/cli/cli/v2/pkg/cmd/issue/argparsetest"
"github.com/cli/cli/v2/pkg/cmdutil"
"github.com/cli/cli/v2/pkg/httpmock"
"github.com/cli/cli/v2/pkg/iostreams"
"github.com/google/shlex"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestNewCmdClose(t *testing.T) {
// Test shared parsing of issue number / URL.
argparsetest.TestArgParsing(t, NewCmdClose)
tests := []struct {
name string
input string
output CloseOptions
wantErr bool
errMsg string
name string
input string
output CloseOptions
expectedBaseRepo ghrepo.Interface
wantErr bool
errMsg string
}{
{
name: "no argument",
input: "",
wantErr: true,
errMsg: "accepts 1 arg(s), received 0",
},
{
name: "issue number",
input: "123",
output: CloseOptions{
SelectorArg: "123",
},
},
{
name: "issue url",
input: "https://github.com/cli/cli/3",
output: CloseOptions{
SelectorArg: "https://github.com/cli/cli/3",
},
},
{
name: "comment",
input: "123 --comment 'closing comment'",
output: CloseOptions{
SelectorArg: "123",
IssueNumber: 123,
Comment: "closing comment",
},
},
@ -54,7 +40,7 @@ func TestNewCmdClose(t *testing.T) {
name: "reason",
input: "123 --reason 'not planned'",
output: CloseOptions{
SelectorArg: "123",
IssueNumber: 123,
Reason: "not planned",
},
},
@ -79,15 +65,24 @@ func TestNewCmdClose(t *testing.T) {
_, err = cmd.ExecuteC()
if tt.wantErr {
assert.Error(t, err)
require.Error(t, err)
assert.Equal(t, tt.errMsg, err.Error())
return
}
assert.NoError(t, err)
assert.Equal(t, tt.output.SelectorArg, gotOpts.SelectorArg)
require.NoError(t, err)
assert.Equal(t, tt.output.IssueNumber, gotOpts.IssueNumber)
assert.Equal(t, tt.output.Comment, gotOpts.Comment)
assert.Equal(t, tt.output.Reason, gotOpts.Reason)
if tt.expectedBaseRepo != nil {
baseRepo, err := gotOpts.BaseRepo()
require.NoError(t, err)
require.True(
t,
ghrepo.IsSame(tt.expectedBaseRepo, baseRepo),
"expected base repo %+v, got %+v", tt.expectedBaseRepo, baseRepo,
)
}
})
}
}
@ -104,7 +99,7 @@ func TestCloseRun(t *testing.T) {
{
name: "close issue by number",
opts: &CloseOptions{
SelectorArg: "13",
IssueNumber: 13,
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(
@ -128,7 +123,7 @@ func TestCloseRun(t *testing.T) {
{
name: "close issue with comment",
opts: &CloseOptions{
SelectorArg: "13",
IssueNumber: 13,
Comment: "closing comment",
},
httpStubs: func(reg *httpmock.Registry) {
@ -164,7 +159,7 @@ func TestCloseRun(t *testing.T) {
{
name: "close issue with reason",
opts: &CloseOptions{
SelectorArg: "13",
IssueNumber: 13,
Reason: "not planned",
Detector: &fd.EnabledDetectorMock{},
},
@ -192,7 +187,7 @@ func TestCloseRun(t *testing.T) {
{
name: "close issue with reason when reason is not supported",
opts: &CloseOptions{
SelectorArg: "13",
IssueNumber: 13,
Reason: "not planned",
Detector: &fd.DisabledDetectorMock{},
},
@ -219,7 +214,7 @@ func TestCloseRun(t *testing.T) {
{
name: "issue already closed",
opts: &CloseOptions{
SelectorArg: "13",
IssueNumber: 13,
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(
@ -236,7 +231,7 @@ func TestCloseRun(t *testing.T) {
{
name: "issues disabled",
opts: &CloseOptions{
SelectorArg: "13",
IssueNumber: 13,
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(

10
pkg/cmd/issue/shared/lookup.go
View file

@ -24,7 +24,7 @@ func ParseIssuesFromArgs(args []string) ([]int, o.Option[ghrepo.Interface], erro
for i, arg := range args {
// For each argument, parse the issue number and an optional repo
issueNumber, issueRepo, err := parseIssueFromArg(arg)
issueNumber, issueRepo, err := ParseIssueFromArg(arg)
if err != nil {
return nil, o.None[ghrepo.Interface](), err
}
@ -53,7 +53,7 @@ func ParseIssuesFromArgs(args []string) ([]int, o.Option[ghrepo.Interface], erro
return issueNumbers, repo, nil
}
func parseIssueFromArg(arg string) (int, o.Option[ghrepo.Interface], error) {
func ParseIssueFromArg(arg string) (int, o.Option[ghrepo.Interface], error) {
issueLocator := tryParseIssueFromURL(arg)
if issueLocator, present := issueLocator.Value(); present {
return issueLocator.issueNumber, o.Some(issueLocator.repo), nil
@ -111,7 +111,7 @@ func IssueFromArgWithFields(httpClient *http.Client, baseRepoFn func() (ghrepo.I
}
}
issue, err := findIssueOrPR(httpClient, baseRepo, issueNumber, fields)
issue, err := FindIssueOrPR(httpClient, baseRepo, issueNumber, fields)
return issue, baseRepo, err
}
@ -165,7 +165,7 @@ func FindIssuesOrPRs(httpClient *http.Client, repo ghrepo.Interface, issueNumber
for _, num := range issueNumbers {
issueNumber := num
g.Go(func() error {
issue, err := findIssueOrPR(httpClient, repo, issueNumber, fields)
issue, err := FindIssueOrPR(httpClient, repo, issueNumber, fields)
if err != nil {
return err
}
@ -190,7 +190,7 @@ func FindIssuesOrPRs(httpClient *http.Client, repo ghrepo.Interface, issueNumber
return issues, nil
}
func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) {
func FindIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) {
fieldSet := set.NewStringSet()
fieldSet.AddValues(fields)
if fieldSet.Contains("stateReason") {