non-interactive gh pr review by vilmibm · Pull Request #876 · cli/cli

vilmibm · 2020-05-07T01:06:52Z

This PR implements part of the lightweight reviewing (scenario B) designed here: https://docs.google.com/document/d/1lk4C5nqNWuOHxy0CDQKLbbrl6GRm4Y3f3FU4xTXgdhI/edit specifically with using boolean flags + a --body string flag

For now it's just the non-interactive component; given that this PR was getting a little long I'm doing the survey case in a follow-up.

examples:

gh pr review -a                               # mark the current branch's pull request as approved
gh pr review -c -b "interesting"              # comment on the current branch's pull request
gh pr review 123 -r -b"needs more ascii art"  # request changes on pull request 123

Related to #434

mislav

It looks like, right now, a review without comment is added like so gh pr review --approve and the review with comment is gh pr review --approve='ship it!'. Since string flags that are optional are both tricky to implement (requires some flags juggling) and tricky for our users to understand, could we consider passing the comment body in a similar manner as to issue/pr create? e.g.

gh pr review --approve - wizard offering a text editor
gh pr review --approve --body 'ole' - passed body skips the wizard

mislav · 2020-05-07T15:50:11Z

api/queries_pr.go


-	"github.com/cli/cli/internal/ghrepo"
 	"github.com/shurcooL/githubv4"
+


Nit: removing this blank line would probably group & sort import statements correctly again, ironing out this diff since it doesn't seem like any imports actually changed

I did this intentionally but I guess subconsciously; it's habit from Python where you split imports into standard library, 3rd party, and internal-to-project paragraphs.

mislav · 2020-05-07T15:53:57Z

api/queries_pr.go

+	body := githubv4.String(input.Body)
+
+	gqlInput := githubv4.AddPullRequestReviewInput{
+		PullRequestID: pr.ID,


This made me realize that passing an ID as a string works in a mutation without casting it to a githubv4.ID, which is mildly surprising because githubv4 requires all other parameters be cast to a specific githubv4.* type 🤔

it surprised me, too; I did it because we're doing it elsewhere in the codebase.

mislav · 2020-05-07T15:58:21Z

command/pr_review.go

+	}
+
+	if found != 1 {
+		return nil, errors.New("need exactly one of approve, request-changes, or comment")


Could we add -- so it clear that these are flags?

Suggested change

return nil, errors.New("need exactly one of approve, request-changes, or comment")

return nil, errors.New("need exactly one of --approve, --request-changes, or --comment")

mislav · 2020-05-07T15:58:52Z

command/pr_review.go

+		state = api.ReviewComment
+	}
+
+	if found != 1 {


Nice way to enforce mutually-exclusive flags!

mislav · 2020-05-07T16:08:47Z

command/pr_review.go

+	prArg := ""
+
+	if len(args) == 0 {
+		prNum, _, err := prSelectorForCurrentBranch(ctx, baseRepo)


prNum might be zero in case the specific number for the current branch could not be detected, so then the 2nd argument will have the branch selector for lookup (e.g. mislav:patch-1) that you can then use with PullRequestForBranch

It's a bit boilerplate-y and undocumented, but you need to handle all those cases otherwise the current branch will not get resolved to a PR.

command/pr_review.go

this commit add very basic non-interactive PR reviewing. You can either review the "current" or a passed PR (number or URL) as approved, changes requested, or commented via CLI flags.

mislav

Code looks great to me!

Something that still feels off to me (and that we can perhaps consider iterating on in the PR that adds interactivity) are the flags: --approve, --request-changes, --comment.

Currently all these are registered as boolean-string flags, meaning they can be passed on their own or with a value. However, the server does not allow --request-changes or --comment without a value, so it's moot that the values for these are optional. Furthermore, the mechanism by which we mark their values as optional has them being rendered like so by Cobra, which might be confusing for our users:

Flags:
  -a, --approve string[=" "]           Approve pull request
  -c, --comment string[=" "]           Comment on a pull request
  -r, --request-changes string[=" "]   Request changes on a pull request

Lastly, boolean-string flags can only receive a value through the = character, e.g. --comment='cool story'. That means neither of these will work:

gh pr review --comment 'cool story'
gh pr review -c 'cool story'

They fail with this cryptic error message: accepts at most 1 arg(s), received 2. Users will have to figure out that an = sign was needed instead of the space.

This might be confusing because an equals sign is not required for any other string flags, e.g. these all work:

gh pr create --title 'cool story' -b 'let me tell you a story'

Personally, I would suggest generally avoiding using boolean-string flags and just make the flags be boolean and pass the body value as a separate string flag.

$ gh pr review 123  # interactive
$ gh pr review 123 --approve  # non-interactive
$ gh pr review 123 -r  # failure: body value is required
$ gh pr review 123 -r --body 'this causes a crash'  # non-interactive

/cc @ampinsk

mislav · 2020-05-08T11:29:07Z

command/pr_review.go

+}
+
+var prReviewCmd = &cobra.Command{
+	Use:   "review",


Should we add [{<number> | <url> | <branch>}] to the usage line like we did with other pr commands?

yup, meant to come back to that and forgot.

mislav · 2020-05-08T12:25:37Z

command/pr_review.go

+		body = val
+	}
+
+	if flag == "comment" && (body == " " || len(body) == 0) {


We might consider extending this behavior to --request-changes too, since it's not possible to request changes without leaving a comment either.

Since --approve is the only option where comment body is optional, we could consider marking other flags as always requiring a value.

Ah, good catch. I'll fix that validation during retooling the flags.

mislav · 2020-05-08T12:34:39Z

command/pr_review.go

+	Use:   "review",
+	Short: "TODO",
+	Args:  cobra.MaximumNArgs(1),
+	Long:  "TODO",


Is documentation left to be addressed in follow-up?

"yes" in that I forgot about it :) I can do it now and then augment to talk about the no args mode

vilmibm · 2020-05-08T14:33:11Z

I was initially in favor of the --approve "foo" approach; but given that to make it work you need to do --approve=foo, that the usage generated is confusing to read, and that the other two states shouldn't have a default I'm now thinking that an additional --body/-b string flag (which has been suggested in a number of places) is going to be easier to document and use.

I'm going to retool for that but @ampinsk feel free to redirect me if you think there's a better path.

ampinsk · 2020-05-08T15:21:04Z

Sounds like a good change to me! Go for it @vilmibm

mislav suggested changes May 7, 2020

View reviewed changes

lightest weight reviewing

d81e622

this commit add very basic non-interactive PR reviewing. You can either review the "current" or a passed PR (number or URL) as approved, changes requested, or commented via CLI flags.

vilmibm force-pushed the review branch from 8d4f2f2 to d81e622 Compare May 7, 2020 18:30

vilmibm added 3 commits May 7, 2020 14:27

handle pr for branch case

5dbec25

clarify error msg

8a5290d

fix current branch tests

46522a2

vilmibm changed the title ~~[wip] gh pr review~~ non-interactive gh pr review May 7, 2020

vilmibm marked this pull request as ready for review May 7, 2020 19:52

sigh

c40f064

vilmibm requested review from mislav and probablycorey May 7, 2020 20:00

vilmibm mentioned this pull request May 7, 2020

light interactive pr review #885

Merged

mislav approved these changes May 8, 2020

View reviewed changes

switch to using body flag. small TODOs

6538cca

vilmibm merged commit 2621b85 into master May 8, 2020

mislav deleted the review branch June 25, 2020 14:37

renovate bot mentioned this pull request Oct 26, 2024

chore(deps): update dependency github-cli to v2.89.0 sourcegraph/eslint-config#295

Open

1 task


		"github.com/cli/cli/internal/ghrepo"
		"github.com/shurcooL/githubv4"

	return nil, errors.New("need exactly one of approve, request-changes, or comment")
	return nil, errors.New("need exactly one of --approve, --request-changes, or --comment")

Conversation

vilmibm commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mislav left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vilmibm commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ampinsk commented May 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vilmibm commented May 7, 2020 •

edited

Loading

mislav left a comment •

edited

Loading

vilmibm commented May 8, 2020 •

edited

Loading