fix(discussion create): improve validation and output behavior
- Move non-interactive flag validation to arg parsing stage - Add blank title/body/category validation at flag parsing - Keep interactive blank-title/body checks after prompts - Always print URL to stdout; success message to stderr in TTY mode - Consolidate test cases and add isTTY field Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Babak K. Shandiz
parent
a1fd235755
commit
3850cacb55
2 changed files with 66 additions and 73 deletions
|
|
@ -57,6 +57,22 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co
|
|||
Args: cmdutil.NoArgsQuoteReminder,
|
||||
RunE: func(cmd *cobra.Command, args []string) error {
|
||||
opts.BaseRepo = f.BaseRepo
|
||||
|
||||
if opts.Title != "" && strings.TrimSpace(opts.Title) == "" {
|
||||
return cmdutil.FlagErrorf("title cannot be blank")
|
||||
}
|
||||
if opts.Body != "" && strings.TrimSpace(opts.Body) == "" {
|
||||
return cmdutil.FlagErrorf("body cannot be blank")
|
||||
}
|
||||
if opts.Category != "" && strings.TrimSpace(opts.Category) == "" {
|
||||
return cmdutil.FlagErrorf("category cannot be blank")
|
||||
}
|
||||
|
||||
needsInput := opts.Title == "" || opts.Category == "" || opts.Body == ""
|
||||
if needsInput && !opts.IO.CanPrompt() {
|
||||
return cmdutil.FlagErrorf("--title, --body, and --category are required when not running interactively")
|
||||
}
|
||||
|
||||
if runF != nil {
|
||||
return runF(opts)
|
||||
}
|
||||
|
|
@ -83,36 +99,19 @@ func createRun(opts *CreateOptions) error {
|
|||
return err
|
||||
}
|
||||
|
||||
interactive := opts.IO.CanPrompt()
|
||||
|
||||
if !interactive {
|
||||
if opts.Title == "" {
|
||||
return cmdutil.FlagErrorf("--title required when not running interactively")
|
||||
}
|
||||
if opts.Category == "" {
|
||||
return cmdutil.FlagErrorf("--category required when not running interactively")
|
||||
}
|
||||
if opts.Body == "" {
|
||||
return cmdutil.FlagErrorf("--body required when not running interactively")
|
||||
}
|
||||
}
|
||||
|
||||
categories, err := c.ListCategories(repo)
|
||||
if err != nil {
|
||||
return fmt.Errorf("fetching categories: %w", err)
|
||||
}
|
||||
|
||||
if opts.Title == "" {
|
||||
if !interactive {
|
||||
return cmdutil.FlagErrorf("--title required when not running interactively")
|
||||
}
|
||||
opts.Title, err = opts.Prompter.Input("Discussion title", "")
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
if strings.TrimSpace(opts.Title) == "" {
|
||||
return cmdutil.FlagErrorf("title cannot be blank")
|
||||
if strings.TrimSpace(opts.Title) == "" {
|
||||
return fmt.Errorf("title cannot be blank")
|
||||
}
|
||||
}
|
||||
|
||||
var category *client.DiscussionCategory
|
||||
|
|
@ -122,9 +121,6 @@ func createRun(opts *CreateOptions) error {
|
|||
return err
|
||||
}
|
||||
} else {
|
||||
if !interactive {
|
||||
return cmdutil.FlagErrorf("--category required when not running interactively")
|
||||
}
|
||||
names := make([]string, len(categories))
|
||||
for i, cat := range categories {
|
||||
names[i] = cat.Name
|
||||
|
|
@ -137,13 +133,13 @@ func createRun(opts *CreateOptions) error {
|
|||
}
|
||||
|
||||
if opts.Body == "" {
|
||||
if !interactive {
|
||||
return cmdutil.FlagErrorf("--body required when not running interactively")
|
||||
}
|
||||
opts.Body, err = opts.Prompter.MarkdownEditor("Discussion body", "", false)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if strings.TrimSpace(opts.Body) == "" {
|
||||
return fmt.Errorf("body cannot be blank")
|
||||
}
|
||||
}
|
||||
|
||||
input := client.CreateDiscussionInput{
|
||||
|
|
@ -160,11 +156,10 @@ func createRun(opts *CreateOptions) error {
|
|||
|
||||
if opts.IO.IsStdoutTTY() {
|
||||
cs := opts.IO.ColorScheme()
|
||||
fmt.Fprintf(opts.IO.Out, "%s Created discussion #%d: %s\n",
|
||||
cs.SuccessIcon(), discussion.Number, discussion.URL)
|
||||
} else {
|
||||
fmt.Fprintln(opts.IO.Out, discussion.URL)
|
||||
fmt.Fprintf(opts.IO.ErrOut, "%s Created discussion #%d\n",
|
||||
cs.SuccessIcon(), discussion.Number)
|
||||
}
|
||||
fmt.Fprintln(opts.IO.Out, discussion.URL)
|
||||
|
||||
return nil
|
||||
}
|
||||
|
|
|
|||
|
|
@ -35,41 +35,65 @@ func TestNewCmdCreate(t *testing.T) {
|
|||
tests := []struct {
|
||||
name string
|
||||
args string
|
||||
isTTY bool
|
||||
wantOpts CreateOptions
|
||||
wantErr string
|
||||
}{
|
||||
{
|
||||
name: "no flags",
|
||||
args: "",
|
||||
isTTY: true,
|
||||
wantOpts: CreateOptions{},
|
||||
},
|
||||
{
|
||||
name: "title flag",
|
||||
args: "--title 'My question'",
|
||||
wantOpts: CreateOptions{
|
||||
Title: "My question",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "all flags",
|
||||
args: "--title 'My question' --body 'Details' --category 'Q&A' --label bug",
|
||||
name: "all flags",
|
||||
args: "--title 'My question' --body 'Details' --category 'Q&A' --label bug,enhancement",
|
||||
isTTY: true,
|
||||
wantOpts: CreateOptions{
|
||||
Title: "My question",
|
||||
Body: "Details",
|
||||
Category: "Q&A",
|
||||
Labels: []string{"bug"},
|
||||
Labels: []string{"bug", "enhancement"},
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "extra args",
|
||||
args: "extra",
|
||||
isTTY: true,
|
||||
wantErr: "unknown argument",
|
||||
},
|
||||
{
|
||||
name: "missing required flags non-interactively",
|
||||
args: "--title 'My question'",
|
||||
isTTY: false,
|
||||
wantErr: "--title, --body, and --category are required when not running interactively",
|
||||
},
|
||||
{
|
||||
name: "blank title",
|
||||
args: "--title ' '",
|
||||
isTTY: true,
|
||||
wantErr: "title cannot be blank",
|
||||
},
|
||||
{
|
||||
name: "blank category",
|
||||
args: "--category ' '",
|
||||
isTTY: true,
|
||||
wantErr: "category cannot be blank",
|
||||
},
|
||||
{
|
||||
name: "blank body",
|
||||
args: "--body ' '",
|
||||
isTTY: true,
|
||||
wantErr: "body cannot be blank",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
f := &cmdutil.Factory{}
|
||||
ios, _, _, _ := iostreams.Test()
|
||||
ios.SetStdinTTY(tt.isTTY)
|
||||
ios.SetStdoutTTY(tt.isTTY)
|
||||
f := &cmdutil.Factory{IOStreams: ios}
|
||||
var capturedOpts *CreateOptions
|
||||
cmd := NewCmdCreate(f, func(opts *CreateOptions) error {
|
||||
capturedOpts = opts
|
||||
|
|
@ -145,33 +169,6 @@ func TestCreateRun_nonInteractive(t *testing.T) {
|
|||
},
|
||||
wantOut: "https://github.com/OWNER/REPO/discussions/5\n",
|
||||
},
|
||||
{
|
||||
name: "missing title returns error",
|
||||
opts: CreateOptions{
|
||||
Body: "Details",
|
||||
Category: "General",
|
||||
},
|
||||
setupMock: func(m *client.DiscussionClientMock) {},
|
||||
wantErr: "--title required when not running interactively",
|
||||
},
|
||||
{
|
||||
name: "missing category returns error",
|
||||
opts: CreateOptions{
|
||||
Title: "My question",
|
||||
Body: "Details",
|
||||
},
|
||||
setupMock: func(m *client.DiscussionClientMock) {},
|
||||
wantErr: "--category required when not running interactively",
|
||||
},
|
||||
{
|
||||
name: "missing body returns error",
|
||||
opts: CreateOptions{
|
||||
Title: "My question",
|
||||
Category: "General",
|
||||
},
|
||||
setupMock: func(m *client.DiscussionClientMock) {},
|
||||
wantErr: "--body required when not running interactively",
|
||||
},
|
||||
{
|
||||
name: "unknown category returns error",
|
||||
opts: CreateOptions{
|
||||
|
|
@ -245,7 +242,7 @@ func TestCreateRun_nonInteractive(t *testing.T) {
|
|||
}
|
||||
|
||||
func TestCreateRun_tty(t *testing.T) {
|
||||
ios, _, stdout, _ := iostreams.Test()
|
||||
ios, _, stdout, stderr := iostreams.Test()
|
||||
ios.SetStdoutTTY(true)
|
||||
ios.SetStdinTTY(true)
|
||||
|
||||
|
|
@ -284,13 +281,13 @@ func TestCreateRun_tty(t *testing.T) {
|
|||
|
||||
err := createRun(opts)
|
||||
require.NoError(t, err)
|
||||
assert.Contains(t, stdout.String(), "Created discussion #5")
|
||||
assert.Contains(t, stdout.String(), "https://github.com/OWNER/REPO/discussions/5")
|
||||
assert.Contains(t, stderr.String(), "Created discussion #5")
|
||||
assert.Equal(t, "https://github.com/OWNER/REPO/discussions/5\n", stdout.String())
|
||||
}
|
||||
|
||||
func TestCreateRun_tty_partialFlags(t *testing.T) {
|
||||
// Title and body provided, category via prompt
|
||||
ios, _, stdout, _ := iostreams.Test()
|
||||
ios, _, stdout, stderr := iostreams.Test()
|
||||
ios.SetStdoutTTY(true)
|
||||
ios.SetStdinTTY(true)
|
||||
|
||||
|
|
@ -323,7 +320,8 @@ func TestCreateRun_tty_partialFlags(t *testing.T) {
|
|||
|
||||
err := createRun(opts)
|
||||
require.NoError(t, err)
|
||||
assert.Contains(t, stdout.String(), "Created discussion #5")
|
||||
assert.Contains(t, stderr.String(), "Created discussion #5")
|
||||
assert.Equal(t, "https://github.com/OWNER/REPO/discussions/5\n", stdout.String())
|
||||
}
|
||||
|
||||
func TestCreateRun_tty_blankTitle(t *testing.T) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue