Issue edit early arg parsing

This commit is contained in:
William Martin 2025-04-16 13:39:34 +02:00
parent 0ef0c42665
commit aaddcb019d
4 changed files with 298 additions and 159 deletions

View file

@ -28,7 +28,7 @@ type EditOptions struct {
EditFieldsSurvey func(prShared.EditPrompter, *prShared.Editable, string) error
FetchOptions func(*api.Client, ghrepo.Interface, *prShared.Editable) error
SelectorArgs []string
IssueNumbers []int
Interactive bool
prShared.Editable
@ -69,10 +69,22 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman
`),
Args: cobra.MinimumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
// support `-R, --repo` override
opts.BaseRepo = f.BaseRepo
issueNumbers, baseRepo, err := shared.ParseIssuesFromArgs(args)
if err != nil {
return err
}
opts.SelectorArgs = args
// 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.IssueNumbers = issueNumbers
flags := cmd.Flags()
@ -134,7 +146,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman
return cmdutil.FlagErrorf("field to edit flag required when not running interactively")
}
if opts.Interactive && len(opts.SelectorArgs) > 1 {
if opts.Interactive && len(opts.IssueNumbers) > 1 {
return cmdutil.FlagErrorf("multiple issues cannot be edited interactively")
}
@ -167,6 +179,11 @@ func editRun(opts *EditOptions) error {
return err
}
baseRepo, err := opts.BaseRepo()
if err != nil {
return err
}
// Prompt the user which fields they'd like to edit.
editable := opts.Editable
if opts.Interactive {
@ -192,7 +209,7 @@ func editRun(opts *EditOptions) error {
}
// Get all specified issues and make sure they are within the same repo.
issues, repo, err := shared.IssuesFromArgsWithFields(httpClient, opts.BaseRepo, opts.SelectorArgs, lookupFields)
issues, err := shared.FindIssuesOrPRs(httpClient, baseRepo, opts.IssueNumbers, lookupFields)
if err != nil {
return err
}
@ -200,7 +217,7 @@ func editRun(opts *EditOptions) error {
// Fetch editable shared fields once for all issues.
apiClient := api.NewClientFromHTTP(httpClient)
opts.IO.StartProgressIndicatorWithLabel("Fetching repository information")
err = opts.FetchOptions(apiClient, repo, &editable)
err = opts.FetchOptions(apiClient, baseRepo, &editable)
opts.IO.StopProgressIndicator()
if err != nil {
return err
@ -250,7 +267,7 @@ func editRun(opts *EditOptions) error {
go func(issue *api.Issue) {
defer g.Done()
err := prShared.UpdateIssue(httpClient, repo, issue.ID, issue.IsPullRequest(), editable)
err := prShared.UpdateIssue(httpClient, baseRepo, issue.ID, issue.IsPullRequest(), editable)
if err != nil {
failedIssueChan <- fmt.Sprintf("failed to update %s: %s", issue.URL, err)
return

View file

@ -26,11 +26,12 @@ func TestNewCmdEdit(t *testing.T) {
require.NoError(t, err)
tests := []struct {
name string
input string
stdin string
output EditOptions
wantsErr bool
name string
input string
stdin string
output EditOptions
expectedBaseRepo ghrepo.Interface
wantsErr bool
}{
{
name: "no argument",
@ -42,7 +43,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "issue number argument",
input: "23",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Interactive: true,
},
wantsErr: false,
@ -51,7 +52,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "title flag",
input: "23 --title test",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Title: prShared.EditableString{
Value: "test",
@ -65,7 +66,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "body flag",
input: "23 --body test",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Body: prShared.EditableString{
Value: "test",
@ -80,7 +81,7 @@ func TestNewCmdEdit(t *testing.T) {
input: "23 --body-file -",
stdin: "this is on standard input",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Body: prShared.EditableString{
Value: "this is on standard input",
@ -94,7 +95,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "body from file",
input: fmt.Sprintf("23 --body-file '%s'", tmpFile),
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Body: prShared.EditableString{
Value: "a body from file",
@ -113,7 +114,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "add-assignee flag",
input: "23 --add-assignee monalisa,hubot",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Assignees: prShared.EditableSlice{
Add: []string{"monalisa", "hubot"},
@ -127,7 +128,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "remove-assignee flag",
input: "23 --remove-assignee monalisa,hubot",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Assignees: prShared.EditableSlice{
Remove: []string{"monalisa", "hubot"},
@ -141,7 +142,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "add-label flag",
input: "23 --add-label feature,TODO,bug",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Labels: prShared.EditableSlice{
Add: []string{"feature", "TODO", "bug"},
@ -155,7 +156,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "remove-label flag",
input: "23 --remove-label feature,TODO,bug",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Labels: prShared.EditableSlice{
Remove: []string{"feature", "TODO", "bug"},
@ -169,7 +170,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "add-project flag",
input: "23 --add-project Cleanup,Roadmap",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Projects: prShared.EditableProjects{
EditableSlice: prShared.EditableSlice{
@ -185,7 +186,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "remove-project flag",
input: "23 --remove-project Cleanup,Roadmap",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Projects: prShared.EditableProjects{
EditableSlice: prShared.EditableSlice{
@ -201,7 +202,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "milestone flag",
input: "23 --milestone GA",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Milestone: prShared.EditableString{
Value: "GA",
@ -215,7 +216,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "remove-milestone flag",
input: "23 --remove-milestone",
output: EditOptions{
SelectorArgs: []string{"23"},
IssueNumbers: []int{23},
Editable: prShared.Editable{
Milestone: prShared.EditableString{
Value: "",
@ -234,7 +235,7 @@ func TestNewCmdEdit(t *testing.T) {
name: "add label to multiple issues",
input: "23 34 --add-label bug",
output: EditOptions{
SelectorArgs: []string{"23", "34"},
IssueNumbers: []int{23, 34},
Editable: prShared.Editable{
Labels: prShared.EditableSlice{
Add: []string{"bug"},
@ -244,6 +245,31 @@ func TestNewCmdEdit(t *testing.T) {
},
wantsErr: false,
},
{
name: "argument is hash prefixed number",
// Escaping is required here to avoid what I think is shellex treating it as a comment.
input: "\\#23",
output: EditOptions{
IssueNumbers: []int{23},
Interactive: true,
},
wantsErr: false,
},
{
name: "argument is a URL",
input: "https://github.com/cli/cli/issues/23",
output: EditOptions{
IssueNumbers: []int{23},
Interactive: true,
},
expectedBaseRepo: ghrepo.New("cli", "cli"),
wantsErr: false,
},
{
name: "URL arguments parse as different repos",
input: "https://github.com/cli/cli/issues/23 https://github.com/cli/go-gh/issues/23",
wantsErr: true,
},
{
name: "interactive multiple issues",
input: "23 34",
@ -282,14 +308,23 @@ func TestNewCmdEdit(t *testing.T) {
_, err = cmd.ExecuteC()
if tt.wantsErr {
assert.Error(t, err)
require.Error(t, err)
return
}
assert.NoError(t, err)
assert.Equal(t, tt.output.SelectorArgs, gotOpts.SelectorArgs)
require.NoError(t, err)
assert.Equal(t, tt.output.IssueNumbers, gotOpts.IssueNumbers)
assert.Equal(t, tt.output.Interactive, gotOpts.Interactive)
assert.Equal(t, tt.output.Editable, gotOpts.Editable)
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,
)
}
})
}
}
@ -306,7 +341,7 @@ func Test_editRun(t *testing.T) {
{
name: "non-interactive",
input: &EditOptions{
SelectorArgs: []string{"123"},
IssueNumbers: []int{123},
Interactive: false,
Editable: prShared.Editable{
Title: prShared.EditableString{
@ -359,7 +394,7 @@ func Test_editRun(t *testing.T) {
{
name: "non-interactive multiple issues",
input: &EditOptions{
SelectorArgs: []string{"456", "123"},
IssueNumbers: []int{456, 123},
Interactive: false,
Editable: prShared.Editable{
Assignees: prShared.EditableSlice{
@ -409,7 +444,7 @@ func Test_editRun(t *testing.T) {
{
name: "non-interactive multiple issues with fetch failures",
input: &EditOptions{
SelectorArgs: []string{"123", "9999"},
IssueNumbers: []int{123, 9999},
Interactive: false,
Editable: prShared.Editable{
Assignees: prShared.EditableSlice{
@ -454,7 +489,7 @@ func Test_editRun(t *testing.T) {
{
name: "non-interactive multiple issues with update failures",
input: &EditOptions{
SelectorArgs: []string{"123", "456"},
IssueNumbers: []int{123, 456},
Interactive: false,
Editable: prShared.Editable{
Assignees: prShared.EditableSlice{
@ -524,7 +559,7 @@ func Test_editRun(t *testing.T) {
{
name: "interactive",
input: &EditOptions{
SelectorArgs: []string{"123"},
IssueNumbers: []int{123},
Interactive: true,
FieldsToEditSurvey: func(p prShared.EditPrompter, eo *prShared.Editable) error {
eo.Title.Edited = true

View file

@ -13,10 +13,89 @@ import (
"github.com/cli/cli/v2/api"
fd "github.com/cli/cli/v2/internal/featuredetection"
"github.com/cli/cli/v2/internal/ghrepo"
o "github.com/cli/cli/v2/pkg/option"
"github.com/cli/cli/v2/pkg/set"
"golang.org/x/sync/errgroup"
)
func ParseIssuesFromArgs(args []string) ([]int, o.Option[ghrepo.Interface], error) {
var repo o.Option[ghrepo.Interface]
issueNumbers := make([]int, len(args))
for i, arg := range args {
// For each argument, parse the issue number and an optional repo
issueNumber, issueRepo, err := parseIssueFromArg(arg)
if err != nil {
return nil, o.None[ghrepo.Interface](), err
}
// if this is our first issue repo found, then we need to set it
if repo.IsNone() {
repo = issueRepo
}
// if there is an issue repo returned, then we need to check if it is the same as the previous one
if issueRepo.IsSome() && repo.IsSome() {
// Unwraps are safe because we've checked for presence above
if !ghrepo.IsSame(repo.Unwrap(), issueRepo.Unwrap()) {
return nil, o.None[ghrepo.Interface](), fmt.Errorf(
"multiple issues must be in same repo: found %q, expected %q",
ghrepo.FullName(issueRepo.Unwrap()),
ghrepo.FullName(repo.Unwrap()),
)
}
}
// add the issue number to the list
issueNumbers[i] = issueNumber
}
return issueNumbers, repo, nil
}
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
}
issueNumber, err := strconv.Atoi(strings.TrimPrefix(arg, "#"))
if err != nil {
return 0, o.None[ghrepo.Interface](), fmt.Errorf("invalid issue format: %q", arg)
}
return issueNumber, o.None[ghrepo.Interface](), nil
}
type issueLocator struct {
issueNumber int
repo ghrepo.Interface
}
// tryParseIssueFromURL tries to parse an issue number and repo from a URL.
func tryParseIssueFromURL(maybeURL string) o.Option[issueLocator] {
u, err := url.Parse(maybeURL)
if err != nil {
return o.None[issueLocator]()
}
if u.Scheme != "https" && u.Scheme != "http" {
return o.None[issueLocator]()
}
m := issueURLRE.FindStringSubmatch(u.Path)
if m == nil {
return o.None[issueLocator]()
}
repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname())
issueNumber, _ := strconv.Atoi(m[3])
return o.Some(issueLocator{
issueNumber: issueNumber,
repo: repo,
})
}
// IssueFromArgWithFields loads an issue or pull request with the specified fields. If some of the fields
// could not be fetched by GraphQL, this returns a non-nil issue and a *PartialLoadError.
func IssueFromArgWithFields(httpClient *http.Client, baseRepoFn func() (ghrepo.Interface, error), arg string, fields []string) (*api.Issue, ghrepo.Interface, error) {
@ -36,70 +115,6 @@ func IssueFromArgWithFields(httpClient *http.Client, baseRepoFn func() (ghrepo.I
return issue, baseRepo, err
}
// IssuesFromArgsWithFields loads 1 or more issues or pull requests with the specified fields. If some of the fields
// could not be fetched by GraphQL, this returns non-nil issues and a *PartialLoadError.
func IssuesFromArgsWithFields(httpClient *http.Client, baseRepoFn func() (ghrepo.Interface, error), args []string, fields []string) ([]*api.Issue, ghrepo.Interface, error) {
var issuesRepo ghrepo.Interface
issueNumbers := make([]int, 0, len(args))
for _, arg := range args {
issueNumber, baseRepo, err := IssueNumberAndRepoFromArg(arg)
if err != nil {
return nil, nil, err
}
issueNumbers = append(issueNumbers, issueNumber)
if baseRepo == nil {
var err error
if baseRepo, err = baseRepoFn(); err != nil {
return nil, nil, err
}
}
if issuesRepo == nil {
issuesRepo = baseRepo
continue
}
if !ghrepo.IsSame(issuesRepo, baseRepo) {
return nil, nil, fmt.Errorf(
"multiple issues must be in same repo: found %q, expected %q",
ghrepo.FullName(baseRepo),
ghrepo.FullName(issuesRepo),
)
}
}
issuesChan := make(chan *api.Issue, len(args))
g := errgroup.Group{}
for _, num := range issueNumbers {
issueNumber := num
g.Go(func() error {
issue, err := findIssueOrPR(httpClient, issuesRepo, issueNumber, fields)
if err != nil {
return err
}
issuesChan <- issue
return nil
})
}
err := g.Wait()
close(issuesChan)
if err != nil {
return nil, nil, err
}
issues := make([]*api.Issue, 0, len(args))
for issue := range issuesChan {
issues = append(issues, issue)
}
return issues, issuesRepo, nil
}
var issueURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/(?:issues|pull)/(\d+)`)
func issueMetadataFromURL(s string) (int, ghrepo.Interface) {
@ -142,6 +157,39 @@ type PartialLoadError struct {
error
}
// FindIssuesOrPRs loads 1 or more issues or pull requests with the specified fields. If some of the fields
// could not be fetched by GraphQL, this returns non-nil issues and a *PartialLoadError.
func FindIssuesOrPRs(httpClient *http.Client, repo ghrepo.Interface, issueNumbers []int, fields []string) ([]*api.Issue, error) {
issuesChan := make(chan *api.Issue, len(issueNumbers))
g := errgroup.Group{}
for _, num := range issueNumbers {
issueNumber := num
g.Go(func() error {
issue, err := findIssueOrPR(httpClient, repo, issueNumber, fields)
if err != nil {
return err
}
issuesChan <- issue
return nil
})
}
err := g.Wait()
close(issuesChan)
if err != nil {
return nil, err
}
issues := make([]*api.Issue, 0, len(issueNumbers))
for issue := range issuesChan {
issues = append(issues, issue)
}
return issues, nil
}
func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) {
fieldSet := set.NewStringSet()
fieldSet.AddValues(fields)

View file

@ -7,7 +7,9 @@ import (
"github.com/cli/cli/v2/internal/ghrepo"
"github.com/cli/cli/v2/pkg/httpmock"
o "github.com/cli/cli/v2/pkg/option"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestIssueFromArgWithFields(t *testing.T) {
@ -214,53 +216,85 @@ func TestIssueFromArgWithFields(t *testing.T) {
}
}
func TestIssuesFromArgsWithFields(t *testing.T) {
type args struct {
baseRepoFn func() (ghrepo.Interface, error)
selectors []string
}
func TestParseIssuesFromArgs(t *testing.T) {
tests := []struct {
name string
args args
httpStub func(*httpmock.Registry)
wantIssues []int
wantRepo string
wantErr bool
wantErrMsg string
behavior string
args []string
expectedIssueNumbers []int
expectedRepo o.Option[ghrepo.Interface]
expectedErr bool
}{
{
name: "multiple repos",
args: args{
selectors: []string{"1", "https://github.com/OWNER/OTHERREPO/issues/2"},
baseRepoFn: func() (ghrepo.Interface, error) {
return ghrepo.New("OWNER", "REPO"), nil
},
},
httpStub: func(r *httpmock.Registry) {
r.Register(
httpmock.GraphQL(`query IssueByNumber\b`),
httpmock.StringResponse(`{"data":{"repository":{
"hasIssuesEnabled": true,
"issue":{"number":1}
}}}`))
r.Register(
httpmock.GraphQL(`query IssueByNumber\b`),
httpmock.StringResponse(`{"data":{"repository":{
"hasIssuesEnabled": true,
"issue":{"number":2}
}}}`))
},
wantErr: true,
wantErrMsg: "multiple issues must be in same repo",
behavior: "when given issue numbers, returns them with no repo",
args: []string{"1", "2"},
expectedIssueNumbers: []int{1, 2},
expectedRepo: o.None[ghrepo.Interface](),
},
{
name: "multiple issues",
args: args{
selectors: []string{"1", "2"},
baseRepoFn: func() (ghrepo.Interface, error) {
return ghrepo.New("OWNER", "REPO"), nil
},
behavior: "when given # prefixed issue numbers, returns them with no repo",
args: []string{"#1", "#2"},
expectedIssueNumbers: []int{1, 2},
expectedRepo: o.None[ghrepo.Interface](),
},
{
behavior: "when given URLs, returns them with the repo",
args: []string{
"https://github.com/OWNER/REPO/issues/1",
"https://github.com/OWNER/REPO/issues/2",
},
expectedIssueNumbers: []int{1, 2},
expectedRepo: o.Some(ghrepo.New("OWNER", "REPO")),
},
{
behavior: "when given URLs in different repos, errors",
args: []string{
"https://github.com/OWNER/REPO/issues/1",
"https://github.com/OWNER/OTHERREPO/issues/2",
},
expectedErr: true,
},
{
behavior: "when given an unparseable argument, errors",
args: []string{"://"},
expectedErr: true,
},
{
behavior: "when given a URL that isn't an issue or PR url, errors",
args: []string{"https://github.com"},
expectedErr: true,
},
}
for _, tc := range tests {
t.Run(tc.behavior, func(t *testing.T) {
issueNumbers, repo, err := ParseIssuesFromArgs(tc.args)
if tc.expectedErr {
require.Error(t, err)
return
}
require.NoError(t, err)
assert.Equal(t, tc.expectedIssueNumbers, issueNumbers)
assert.Equal(t, tc.expectedRepo, repo)
})
}
}
func TestFindIssuesOrPRs(t *testing.T) {
tests := []struct {
name string
issueNumbers []int
baseRepo ghrepo.Interface
httpStub func(*httpmock.Registry)
wantIssueNumbers []int
wantErr bool
}{
{
name: "multiple issues",
issueNumbers: []int{1, 2},
baseRepo: ghrepo.New("OWNER", "REPO"),
httpStub: func(r *httpmock.Registry) {
r.Register(
httpmock.GraphQL(`query IssueByNumber\b`),
@ -275,43 +309,48 @@ func TestIssuesFromArgsWithFields(t *testing.T) {
"issue":{"number":2}
}}}`))
},
wantIssues: []int{1, 2},
wantRepo: "https://github.com/OWNER/REPO",
wantIssueNumbers: []int{1, 2},
},
{
name: "any find error results in total error",
issueNumbers: []int{1, 2},
baseRepo: ghrepo.New("OWNER", "REPO"),
httpStub: func(r *httpmock.Registry) {
r.Register(
httpmock.GraphQL(`query IssueByNumber\b`),
httpmock.StringResponse(`{"data":{"repository":{
"hasIssuesEnabled": true,
"issue":{"number":1}
}}}`))
r.Register(
httpmock.GraphQL(`query IssueByNumber\b`),
httpmock.StatusStringResponse(500, "internal server error"))
},
wantErr: true,
},
}
for _, tt := range tests {
if !tt.wantErr && len(tt.args.selectors) != len(tt.wantIssues) {
t.Fatal("number of selectors and issues not equal")
}
t.Run(tt.name, func(t *testing.T) {
reg := &httpmock.Registry{}
if tt.httpStub != nil {
tt.httpStub(reg)
}
httpClient := &http.Client{Transport: reg}
issues, repo, err := IssuesFromArgsWithFields(httpClient, tt.args.baseRepoFn, tt.args.selectors, []string{"number"})
issues, err := FindIssuesOrPRs(httpClient, tt.baseRepo, tt.issueNumbers, []string{"number"})
if (err != nil) != tt.wantErr {
t.Errorf("IssuesFromArgsWithFields() error = %v, wantErr %v", err, tt.wantErr)
t.Errorf("FindIssuesOrPRs() error = %v, wantErr %v", err, tt.wantErr)
if issues == nil {
return
}
}
if tt.wantErr {
assert.Error(t, err)
assert.ErrorContains(t, err, tt.wantErrMsg)
require.Error(t, err)
return
}
assert.NoError(t, err)
require.NoError(t, err)
for i := range issues {
assert.Contains(t, tt.wantIssues, issues[i].Number)
}
if repo != nil {
repoURL := ghrepo.GenerateRepoURL(repo, "")
if repoURL != tt.wantRepo {
t.Errorf("want repo %s, got %s", tt.wantRepo, repoURL)
}
} else if tt.wantRepo != "" {
t.Errorf("want repo %sw, got nil", tt.wantRepo)
assert.Contains(t, tt.wantIssueNumbers, issues[i].Number)
}
})
}