Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

fix: respect edit-last and create-if-none in non-interactive mode - #10582

#10582
Closed
GriceTurrble wants to merge 1 commit into
cli:trunkcli/cli:trunkfrom
GriceTurrble:trunkCopy head branch name to clipboard
Closed

fix: respect edit-last and create-if-none in non-interactive mode#10582
GriceTurrble wants to merge 1 commit into
cli:trunkcli/cli:trunkfrom
GriceTurrble:trunkCopy head branch name to clipboard

Conversation

@GriceTurrble

@GriceTurrble GriceTurrble commented Mar 11, 2025

Copy link
Copy Markdown

Fixes #10580

Previous changes made in #10427 created a behavioral change when using i.e. gh pr comment --edit-last in non-interactive mode, which was not properly accounted for. Ideally, using --edit-last should error out if the newer --create-if-none flag is not used, instead of creating a new comment anyway.

Assistance on updating tests would be appreciated, this is my first time editing Golang code. :)

Edit: tested locally, should be working now. And the changes are much smaller than originally pushed. 🙂

@GriceTurrble
GriceTurrble requested a review from a team as a code owner March 11, 2025 17:12
@GriceTurrble
GriceTurrble requested a review from andyfeller March 11, 2025 17:12
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Mar 11, 2025
…, instead of creating new comments automatically
@andyfeller

andyfeller commented Mar 12, 2025

Copy link
Copy Markdown
Contributor

@GriceTurrble : thank you for working to improve the GitHub CLI! ❤️

Generally, we wait until an issue has been fully triaged with acceptance criteria and marked as help wanted before accepting contributions. Additionally, any contributions must have accompanying tests to ensure gh commands work as expected.

I might also suggest that the current conditional logic structure of CommentableRun isn't really easy to understand and should be flattened somewhat.

Current logic

func CommentableRun(opts *CommentableOptions) error {
commentable, repo, err := opts.RetrieveCommentable()
if err != nil {
return err
}
opts.Host = repo.RepoHost()
if opts.EditLast {
err := updateComment(commentable, opts)
if !errors.Is(err, errNoUserComments) {
return err
}
if opts.Interactive {
if opts.CreateIfNone {
fmt.Fprintln(opts.IO.ErrOut, "No comments found. Creating a new comment.")
} else {
ok, err := opts.ConfirmCreateIfNoneSurvey()
if err != nil {
return err
}
if !ok {
return errNoUserComments
}
}
} else if !opts.CreateIfNone {
return errNoUserComments
}
}
return createComment(commentable, opts)
}

Previous logic

func CommentableRun(opts *CommentableOptions) error {
commentable, repo, err := opts.RetrieveCommentable()
if err != nil {
return err
}
opts.Host = repo.RepoHost()
if opts.EditLast {
return updateComment(commentable, opts)
}
return createComment(commentable, opts)
}

For now, I'm closing this pull request so we can understand exactly what should change here. 🙇

@andyfeller andyfeller closed this Mar 12, 2025
@andyfeller

Copy link
Copy Markdown
Contributor

I'm not saying this is exactly what I would do but want to provide context of what might be easier to understand and manage:

func CommentableRun(opts *CommentableOptions) error {
	commentable, repo, err := opts.RetrieveCommentable()
	if err != nil {
		return err
	}
	opts.Host = repo.RepoHost()

	// Create new comment, bail before complexities of updating the last comment
	if !opts.EditLast {
		return createComment(commentable, opts)
	}

	// Update the last comment, handling success or unexpected errors accordingly
	err := updateComment(commentable, opts)
	if err == nil {
		return nil
	}
	if !errors.Is(err, errNoUserComments) {
		return err
	}

	// Determine whether to create new comment, prompt user if interactive and missing option
	if !opts.CreateIfNone && opts.Interactive {
		opts.CreateIfNone, err = opts.ConfirmCreateIfNoneSurvey()
		if err != nil {
			return err
		}
	}

	// Create new comment because updating the last comment failed due to no user comments
	if opts.CreateIfNone {
		if opts.Interactive {
			fmt.Fprintln(opts.IO.ErrOut, "No comments found. Creating a new comment.")
		}
		return createComment(commentable, opts)
	}

	return errNoUserComments
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external pull request originating outside of the CLI core team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gh pr comment --edit-last now creates if no previous comment

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.