From 3850cacb551012f4a9e28b0262270496db8b2bfb Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Thu, 7 May 2026 08:43:57 +0100 Subject: [PATCH] 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> --- pkg/cmd/discussion/create/create.go | 55 +++++++--------- pkg/cmd/discussion/create/create_test.go | 84 ++++++++++++------------ 2 files changed, 66 insertions(+), 73 deletions(-) diff --git a/pkg/cmd/discussion/create/create.go b/pkg/cmd/discussion/create/create.go index 715e9d7e2..09bfc414a 100644 --- a/pkg/cmd/discussion/create/create.go +++ b/pkg/cmd/discussion/create/create.go @@ -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 } diff --git a/pkg/cmd/discussion/create/create_test.go b/pkg/cmd/discussion/create/create_test.go index 9eede150e..1a413b32e 100644 --- a/pkg/cmd/discussion/create/create_test.go +++ b/pkg/cmd/discussion/create/create_test.go @@ -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) {