From 198487e1666f8800ca6fbf5e351320acd1a50a3f Mon Sep 17 00:00:00 2001 From: yuvrajangadsingh Date: Tue, 10 Mar 2026 18:27:21 +0530 Subject: [PATCH] fix: address review feedback on squash merge commit message - reorder if checks: validate --enable-squash-merge is set before checking the value, and error when --enable-squash-merge=false - use validSquashMsgValues directly in interactive prompt instead of duplicating the slice - use slices.Contains in validateSquashMergeCommitMsg - interpolate const values in Long description instead of hardcoding - add default clause in transformSquashMergeOpts to avoid mutating title/message on unknown input - move optionDiscussions to end of const block with TODO comment - add test for unknown input and --enable-squash-merge=false case --- pkg/cmd/repo/edit/edit.go | 39 +++++++++++++++++----------------- pkg/cmd/repo/edit/edit_test.go | 14 ++++++++++++ 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index 3f75915ef..aff7a5fe1 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "net/http" + "slices" "strings" "time" @@ -47,11 +48,13 @@ const ( optionIssues = "Issues" optionMergeOptions = "Merge Options" optionProjects = "Projects" - optionDiscussions = "Discussions" optionTemplateRepo = "Template Repository" optionTopics = "Topics" optionVisibility = "Visibility" optionWikis = "Wikis" + + // TODO: GitHub Enterprise Server does not support has_discussions yet + // optionDiscussions = "Discussions" ) type EditOptions struct { @@ -132,11 +135,11 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr When the %[1]s--enable-squash-merge%[1]s flag is used, %[1]s--squash-merge-commit-message%[1]s can be used to change the default squash merge commit message behavior: - - %[1]sdefault%[1]s: uses commit title and message for 1 commit, or pull request title and list of commits for 2 or more - - %[1]spr-title%[1]s: uses pull request title - - %[1]spr-title-commits%[1]s: uses pull request title and list of commits - - %[1]spr-title-description%[1]s: uses pull request title and description - `, "`"), + - %[1]s%[2]s%[1]s: uses commit title and message for 1 commit, or pull request title and list of commits for 2 or more + - %[1]s%[3]s%[1]s: uses pull request title + - %[1]s%[4]s%[1]s: uses pull request title and list of commits + - %[1]s%[5]s%[1]s: uses pull request title and description + `, "`", squashMsgDefault, squashMsgPRTitle, squashMsgPRTitleCommits, squashMsgPRTitleDescription), Args: cobra.MaximumNArgs(1), Example: heredoc.Doc(` # Enable issues and wiki @@ -179,12 +182,15 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr } if opts.Edits.squashMergeCommitMsg != nil { - if err := validateSquashMergeCommitMsg(*opts.Edits.squashMergeCommitMsg); err != nil { - return err - } if opts.Edits.EnableSquashMerge == nil { return cmdutil.FlagErrorf("--squash-merge-commit-message requires --enable-squash-merge") } + if !*opts.Edits.EnableSquashMerge { + return cmdutil.FlagErrorf("--squash-merge-commit-message cannot be used when --enable-squash-merge=false") + } + if err := validateSquashMergeCommitMsg(*opts.Edits.squashMergeCommitMsg); err != nil { + return err + } transformSquashMergeOpts(&opts.Edits) } @@ -502,12 +508,7 @@ func interactiveRepoEdit(opts *EditOptions, r *api.Repository) error { } if enableSquashMerge { - squashMsgOptions := []string{ - squashMsgDefault, - squashMsgPRTitle, - squashMsgPRTitleCommits, - squashMsgPRTitleDescription, - } + squashMsgOptions := validSquashMsgValues idx, err := p.Select( "Default squash merge commit message", squashMsgDefault, @@ -684,10 +685,8 @@ func transformSecurityAndAnalysisOpts(opts *EditOptions) *SecurityAndAnalysisInp var validSquashMsgValues = []string{squashMsgDefault, squashMsgPRTitle, squashMsgPRTitleCommits, squashMsgPRTitleDescription} func validateSquashMergeCommitMsg(value string) error { - for _, v := range validSquashMsgValues { - if value == v { - return nil - } + if slices.Contains(validSquashMsgValues, value) { + return nil } return cmdutil.FlagErrorf("invalid value for --squash-merge-commit-message: %q. Valid values are: %s", value, strings.Join(validSquashMsgValues, ", ")) } @@ -712,6 +711,8 @@ func transformSquashMergeOpts(edits *EditRepositoryInput) { case squashMsgPRTitleDescription: title = "PR_TITLE" message = "PR_BODY" + default: + return } edits.SquashMergeCommitTitle = &title edits.SquashMergeCommitMessage = &message diff --git a/pkg/cmd/repo/edit/edit_test.go b/pkg/cmd/repo/edit/edit_test.go index bad2210d8..d4b297a19 100644 --- a/pkg/cmd/repo/edit/edit_test.go +++ b/pkg/cmd/repo/edit/edit_test.go @@ -114,6 +114,11 @@ func TestNewCmdEdit(t *testing.T) { args: "--enable-squash-merge --squash-merge-commit-message blah", wantErr: `invalid value for --squash-merge-commit-message: "blah". Valid values are: default, pr-title, pr-title-commits, pr-title-description`, }, + { + name: "squash merge commit message with enable-squash-merge=false", + args: "--enable-squash-merge=false --squash-merge-commit-message default", + wantErr: "--squash-merge-commit-message cannot be used when --enable-squash-merge=false", + }, } for _, tt := range tests { @@ -972,6 +977,15 @@ func Test_transformSquashMergeOpts(t *testing.T) { } } +func Test_transformSquashMergeOpts_unknownInput(t *testing.T) { + edits := &EditRepositoryInput{ + squashMergeCommitMsg: sp("unknown-value"), + } + transformSquashMergeOpts(edits) + assert.Nil(t, edits.SquashMergeCommitTitle) + assert.Nil(t, edits.SquashMergeCommitMessage) +} + func Test_validateSquashMergeCommitMsg(t *testing.T) { assert.NoError(t, validateSquashMergeCommitMsg("default")) assert.NoError(t, validateSquashMergeCommitMsg("pr-title"))