pr command scriptability improvements #1373

vilmibm · 2020-07-16T21:16:59Z

This PR came out of the research in #940 and consists of several small changes to pr commands that
make them more amenable to scripting.

In this PR, when run unattached to a TTY:

pr create clearly errors if -t or -f are missing
pr create clearly errors if -w is specified
pr list prints more machine readable output, including draft status
pr view prints raw pr body + linewise metadata, including draft status
various human-oriented informational messages are suppressed

Closes #1297
Part of #1082
Part of #939

ampinsk · 2020-07-16T22:00:19Z

@vilmibm thanks for this! Is ready status in addition to overall PR state? I think of draft status as alongside general PR state, so state can be Draft, Open, Merged, Closed and "reviewability" can be inferred from that, but I might be missing some context!

vilmibm · 2020-07-17T01:52:50Z

@ampinsk

I think of draft status as alongside general PR state, so state can be Draft, Open, Merged, Closed and "reviewability" can be inferred from that, but I might be missing some context!

That makes total sense and we're communicating that right now with color (gray for open+draft, green for open+ready, red for closed, purple for merged).

I felt weird for this machine readable output since "draft" is not a PR state that the API can filter by. State is either open, closed, or merged; draft is this weird orthogonal status. I wondered if someone integrating this in a script would expect to handle this data in the same way the API does: where "draft" and "state" are two separate concepts.

If you think it makes sense to collapse those concepts I can report on open+draft PRs as having a DRAFT state instead of OPEN.

I don't have any strong feelings here other than it should be possible in the machine readable state to determine draft status so I'm open to whatever approach.

mislav

If you think it makes sense to collapse those concepts I can report on open+draft PRs as having a DRAFT state instead of OPEN.

I would vote for this approach as well! ✋

mislav · 2020-07-17T18:02:55Z

command/pr.go

+		if !table.IsTTY() {
+			table.AddField(pr.State, nil, nil)
+			table.AddField(prReadyState(&pr), nil, nil)


The two extra columns when output is redirected makes this output to be radically different from the human-readable one. However, I agree that we used to use color to communicate open/closed/merged/draft states and that we now have to communicate that using words as well.

Should we add the same columns to the default terminal output as well, for parity?

Would we consider adding the columns at the end instead of in the middle, to avoid breaking existing scripts? We're in beta, so technically we could make breaking changes, but if could avoid doing that it would be better.

I collapsed "OPEN" and draft status into a combined "DRAFT" and moved the new column to the end of the output 👍

mislav · 2020-07-17T18:04:35Z

command/pr.go

+	if web && !connectedToTerminal(cmd) {
+		return errors.New("--web unsupported when not attached to a tty")


Could we allow --web in scripting scenarios? Since people can set BROWSER environment variable to be the tool of their choice, it might be useful to allow it. Is there some part of --web experience that requires an interactive terminal?

hm; I suppose we could. I think my brain was interpreting "non-tty" as "human not present" but they might want this tool to open things in a browser as part of a larger script.

I'll undo all that here and in the other PRs.

makes sense! My brain was accidentally superimposing "non-tty" with "no human at computer" but naturally that wouldn't be the case in a script that composes gh. I restored this behavior but did suppress the informational printing if not connected to a tty.

mislav · 2020-07-17T18:06:43Z

command/pr_review.go

 	case api.ReviewRequestChanges:
-		fmt.Fprintf(out, "%s Requested changes to pull request #%d\n", utils.Red("+"), pr.Number)
+		fmt.Fprintf(stderr, "%s Requested changes to pull request #%d\n", utils.Red("+"), pr.Number)


Nice that this is now stderr! 🌟

ampinsk · 2020-07-17T18:23:39Z

If you think it makes sense to collapse those concepts I can report on open+draft PRs as having a DRAFT state instead of OPEN.

Yeah, since this is how it's presented in the UI on dotcom I think we're safe to do it this way

This commit improves behavior and error handling for pr commands when run unattached to a tty. - error if pr create and no -t/-f - error if pr create and -w - machine readable pr list - machine readable pr view - various cleanup of informational messages

mislav

Fantastic! Thanks for the hard work

mislav · 2020-07-21T12:19:19Z

command/pr_review.go

@@ -106,15 +109,15 @@ func prReview(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("did not understand desired review action: %w", err)
 	}

-	out := colorableOut(cmd)
+	stderr := cmd.ErrOrStderr()


I think that this should be made "colorable" so Windows can properly output colors on stderr

mislav · 2020-07-21T12:20:47Z

command/pr_test.go

+		regexp.MustCompile(`#32.*New feature.*feature`),
+		regexp.MustCompile(`#29.*Fixed bad bug.*hubot:bug-fix`),
+		regexp.MustCompile(`#28.*Improve documentation.*docs`),


Seeing these regexps makes me think that we could benefit in the future from an assertion matcher that first strips ANSI escape sequences. Then we can assert expected output in an exact manner (i.e. without .*)

yeah, I think that would be wise.

vilmibm added this to Backlog 🗒 in The GitHub CLI via automation Jul 16, 2020

vilmibm requested a review from mislav July 16, 2020 21:17

vilmibm moved this from Backlog 🗒 to Needs review 🤔 in The GitHub CLI Jul 16, 2020

mislav reviewed Jul 17, 2020

View reviewed changes

vilmibm added 2 commits July 20, 2020 13:04

restore nontty + web but suppress informational prints

bffcbce

vilmibm force-pushed the pr-scriptability branch from 1ca5ce8 to b2aca57 Compare July 20, 2020 19:19

vilmibm added 2 commits July 20, 2020 15:55

fix tests

f30f931

collapse open/draft state

fa92848

vilmibm force-pushed the pr-scriptability branch from b2aca57 to fa92848 Compare July 20, 2020 20:55

vilmibm requested a review from mislav July 20, 2020 20:57

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Jul 21, 2020

mislav approved these changes Jul 21, 2020

View reviewed changes

use colorableErr

1137e40

vilmibm merged commit 739d66b into trunk Jul 21, 2020

The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Jul 21, 2020

github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Jul 28, 2020

mislav deleted the pr-scriptability branch August 3, 2020 08:01

This was referenced Aug 14, 2020

cant use gh pr create #1523

Closed

Provide all metadata fields in scripted pr view #1532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pr command scriptability improvements #1373

pr command scriptability improvements #1373

vilmibm commented Jul 16, 2020 •

edited

ampinsk commented Jul 16, 2020

vilmibm commented Jul 17, 2020

mislav left a comment

mislav Jul 17, 2020 •

edited

vilmibm Jul 20, 2020

mislav Jul 17, 2020

vilmibm Jul 17, 2020

vilmibm Jul 20, 2020

mislav Jul 17, 2020

ampinsk commented Jul 17, 2020

mislav left a comment

mislav Jul 21, 2020

vilmibm Jul 21, 2020

mislav Jul 21, 2020

vilmibm Jul 21, 2020

		if web && !connectedToTerminal(cmd) {
		return errors.New("--web unsupported when not attached to a tty")

pr command scriptability improvements #1373

pr command scriptability improvements #1373

Conversation

vilmibm commented Jul 16, 2020 • edited

ampinsk commented Jul 16, 2020

vilmibm commented Jul 17, 2020

mislav left a comment

Choose a reason for hiding this comment

mislav Jul 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ampinsk commented Jul 17, 2020

mislav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vilmibm commented Jul 16, 2020 •

edited

mislav Jul 17, 2020 •

edited