From 36ade42ba30322e18cf6bcf63861cb4a23dece65 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 1 Jul 2020 16:00:12 -0500 Subject: [PATCH 1/3] scriptability improvements: issue commands This commit is part of work to make gh more scriptable. It includes both some general purpose helpers towards this goal as well as improvements to the issue commands. Other commands will follow. - Adds `utils/terminal.go` for finding out about gh's execution environment - introduces `stubTerminal` for either faking being attached to a tty or not during tests - updates issue commands to behave better when not attached to a tty: - issue list doesn't print fuzzy dates - issue list doesn't print header - issue list prints state explicitly - issue create no longer hangs - issue create fails with clear error unless both -t and -b are specified - issue view prints raw issue body - issue view prints metadata in a consistent, linewise format --- command/issue.go | 48 +++++++++++-- command/issue_test.go | 154 +++++++++++++++++++++++++++++++++++++---- command/root.go | 4 ++ command/testing.go | 24 +++++++ utils/color.go | 21 ++---- utils/table_printer.go | 35 ++++------ utils/terminal.go | 44 ++++++++++++ 7 files changed, 273 insertions(+), 57 deletions(-) create mode 100644 utils/terminal.go diff --git a/command/issue.go b/command/issue.go index 67c315a9d..720d81669 100644 --- a/command/issue.go +++ b/command/issue.go @@ -184,8 +184,9 @@ func issueList(cmd *cobra.Command, args []string) error { }) title := listHeader(ghrepo.FullName(baseRepo), "issue", len(listResult.Issues), listResult.TotalCount, hasFilters) - // TODO: avoid printing header if piped to a script - fmt.Fprintf(colorableErr(cmd), "\n%s\n\n", title) + if connectedToTerminal(cmd) { + fmt.Fprintf(colorableErr(cmd), "\n%s\n\n", title) + } out := cmd.OutOrStdout() @@ -278,8 +279,11 @@ func issueView(cmd *cobra.Command, args []string) error { fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", openURL) return utils.OpenInBrowser(openURL) } - out := colorableOut(cmd) - return printIssuePreview(out, issue) + if connectedToTerminal(cmd) { + return printHumanIssuePreview(colorableOut(cmd), issue) + } + + return printRawIssuePreview(cmd.OutOrStdout(), issue) } func issueStateTitleWithColor(state string) string { @@ -306,7 +310,28 @@ func listHeader(repoName string, itemName string, matchCount int, totalMatchCoun return fmt.Sprintf("Showing %d of %s in %s", matchCount, utils.Pluralize(totalMatchCount, itemName), repoName) } -func printIssuePreview(out io.Writer, issue *api.Issue) error { +func printRawIssuePreview(out io.Writer, issue *api.Issue) error { + assignees := issueAssigneeList(*issue) + labels := issueLabelList(*issue) + projects := issueProjectList(*issue) + + // Print empty strings for empty values so the number of metadata lines is always consistent (ie + // so head -n8 can be relied on) + fmt.Fprintf(out, "title:\t%s\n", issue.Title) + fmt.Fprintf(out, "state:\t%s\n", issue.State) + fmt.Fprintf(out, "author:\t%s\n", issue.Author.Login) + fmt.Fprintf(out, "labels:\t%s\n", labels) + fmt.Fprintf(out, "comments:\t%d\n", issue.Comments.TotalCount) + fmt.Fprintf(out, "assignees:\t%s\n", assignees) + fmt.Fprintf(out, "projects:\t%s\n", projects) + fmt.Fprintf(out, "milestone:\t%s\n", issue.Milestone.Title) + + fmt.Fprintln(out, "--") + fmt.Fprintln(out, issue.Body) + return nil +} + +func printHumanIssuePreview(out io.Writer, issue *api.Issue) error { now := time.Now() ago := now.Sub(issue.CreatedAt) @@ -464,6 +489,10 @@ func issueCreate(cmd *cobra.Command, args []string) error { interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) + if interactive && !connectedToTerminal(cmd) { + return fmt.Errorf("can't run non-interactively without both --title and --body") + } + if interactive { var legacyTemplateFile *string if baseOverride == "" { @@ -625,9 +654,16 @@ func printIssues(w io.Writer, prefix string, totalCount int, issues []api.Issue) now := time.Now() ago := now.Sub(issue.UpdatedAt) table.AddField(issueNum, nil, colorFuncForState(issue.State)) + if !table.IsTTY() { + table.AddField(issue.State, nil, nil) + } table.AddField(replaceExcessiveWhitespace(issue.Title), nil, nil) table.AddField(labels, nil, utils.Gray) - table.AddField(utils.FuzzyAgo(ago), nil, utils.Gray) + if table.IsTTY() { + table.AddField(utils.FuzzyAgo(ago), nil, utils.Gray) + } else { + table.AddField(fmt.Sprintf("%d", int(ago.Minutes())), nil, nil) + } table.EndRow() } _ = table.Render() diff --git a/command/issue_test.go b/command/issue_test.go index a3ad56b3a..7ed580c90 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -18,6 +18,7 @@ import ( func TestIssueStatus(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register( @@ -107,8 +108,31 @@ func TestIssueStatus_disabledIssues(t *testing.T) { } } -func TestIssueList(t *testing.T) { +func TestIssueList_nontty(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(false)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + jsonFile, _ := os.Open("../test/fixtures/issueList.json") + defer jsonFile.Close() + http.StubResponse(200, jsonFile) + + output, err := RunCommand("issue list") + if err != nil { + t.Errorf("error running command `issue list`: %v", err) + } + + eq(t, output.Stderr(), "") + test.ExpectLines(t, output.String(), + `1[\t]+number won[\t]+label[\t]+\d+`, + `2[\t]+number too[\t]+label[\t]+\d+`, + `4[\t]+number fore[\t]+label[\t]+\d+`) +} + +func TestIssueList_tty(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register( @@ -125,21 +149,14 @@ Showing 3 of 3 issues in OWNER/REPO `) - expectedIssues := []*regexp.Regexp{ - regexp.MustCompile(`(?m)^1\t.*won`), - regexp.MustCompile(`(?m)^2\t.*too`), - regexp.MustCompile(`(?m)^4\t.*fore`), - } - - for _, r := range expectedIssues { - if !r.MatchString(output.String()) { - t.Errorf("output did not match regexp /%s/\n> output\n%s\n", r, output) - return - } - } + test.ExpectLines(t, output.String(), + "number won", + "number too", + "number fore") } -func TestIssueList_withFlags(t *testing.T) { +func TestIssueList_tty_withFlags(t *testing.T) { + defer stubTerminal(true)() initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -296,7 +313,92 @@ func TestIssueView_web_numberArgWithHash(t *testing.T) { eq(t, url, "https://github.com/OWNER/REPO/issues/123") } -func TestIssueView_Preview(t *testing.T) { +func TestIssueView_nontty_Preview(t *testing.T) { + defer stubTerminal(false)() + tests := map[string]struct { + ownerRepo string + command string + fixture string + expectedOutputs []string + }{ + "Open issue without metadata": { + ownerRepo: "master", + command: "issue view 123", + fixture: "../test/fixtures/issueView_preview.json", + expectedOutputs: []string{ + `title:\tix of coins`, + `state:\tOPEN`, + `comments:\t9`, + `author:\tmarseilles`, + `assignees:`, + `\*\*bold story\*\*`, + }, + }, + "Open issue with metadata": { + ownerRepo: "master", + command: "issue view 123", + fixture: "../test/fixtures/issueView_previewWithMetadata.json", + expectedOutputs: []string{ + `title:\tix of coins`, + `assignees:\tmarseilles, monaco`, + `author:\tmarseilles`, + `state:\tOPEN`, + `comments:\t9`, + `labels:\tone, two, three, four, five`, + `projects:\tProject 1 \(column A\), Project 2 \(column B\), Project 3 \(column C\), Project 4 \(Awaiting triage\)\n`, + `milestone:\tuluru\n`, + `\*\*bold story\*\*`, + }, + }, + "Open issue with empty body": { + ownerRepo: "master", + command: "issue view 123", + fixture: "../test/fixtures/issueView_previewWithEmptyBody.json", + expectedOutputs: []string{ + `title:\tix of coins`, + `state:\tOPEN`, + `author:\tmarseilles`, + `labels:\ttarot`, + }, + }, + "Closed issue": { + ownerRepo: "master", + command: "issue view 123", + fixture: "../test/fixtures/issueView_previewClosedState.json", + expectedOutputs: []string{ + `title:\tix of coins`, + `state:\tCLOSED`, + `\*\*bold story\*\*`, + `author:\tmarseilles`, + `labels:\ttarot`, + `\*\*bold story\*\*`, + }, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + initBlankContext("", "OWNER/REPO", tc.ownerRepo) + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + jsonFile, _ := os.Open(tc.fixture) + defer jsonFile.Close() + http.StubResponse(200, jsonFile) + + output, err := RunCommand(tc.command) + if err != nil { + t.Errorf("error running command `%v`: %v", tc.command, err) + } + + eq(t, output.Stderr(), "") + + test.ExpectLines(t, output.String(), tc.expectedOutputs...) + }) + } +} + +func TestIssueView_tty_Preview(t *testing.T) { + defer stubTerminal(true)() tests := map[string]struct { ownerRepo string command string @@ -448,6 +550,28 @@ func TestIssueView_web_urlArg(t *testing.T) { eq(t, url, "https://github.com/OWNER/REPO/issues/123") } +func TestIssueCreate_nontty_error(t *testing.T) { + defer stubTerminal(false)() + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": true + } } } + `)) + + _, err := RunCommand(`issue create -t hello`) + if err == nil { + t.Fatal("expected error running command `issue create`") + } + + assert.Equal(t, "can't run non-interactively without both --title and --body", err.Error()) + +} + func TestIssueCreate(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() diff --git a/command/root.go b/command/root.go index 3693fd6f0..df607a50d 100644 --- a/command/root.go +++ b/command/root.go @@ -407,3 +407,7 @@ func ExpandAlias(args []string) ([]string, error) { return args[1:], nil } + +func connectedToTerminal(cmd *cobra.Command) bool { + return utils.IsTerminal(cmd.InOrStdin()) && utils.IsTerminal(cmd.OutOrStdout()) +} diff --git a/command/testing.go b/command/testing.go index af713aa29..d9599e9b3 100644 --- a/command/testing.go +++ b/command/testing.go @@ -12,6 +12,7 @@ import ( "github.com/cli/cli/context" "github.com/cli/cli/internal/config" "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/utils" "github.com/google/shlex" "github.com/spf13/pflag" ) @@ -169,3 +170,26 @@ func (s errorStub) Output() ([]byte, error) { func (s errorStub) Run() error { return errors.New(s.message) } + +func stubTerminal(connected bool) func() { + isTerminal := utils.IsTerminal + utils.IsTerminal = func(_ interface{}) bool { + return connected + } + + terminalSize := utils.TerminalSize + if connected { + utils.TerminalSize = func(_ interface{}) (int, int, error) { + return 80, 20, nil + } + } else { + utils.TerminalSize = func(_ interface{}) (int, int, error) { + return 0, 0, fmt.Errorf("terminal connection stubbed to false") + } + } + + return func() { + utils.IsTerminal = isTerminal + utils.TerminalSize = terminalSize + } +} diff --git a/utils/color.go b/utils/color.go index dd9a7d11c..43a3277f9 100644 --- a/utils/color.go +++ b/utils/color.go @@ -5,7 +5,6 @@ import ( "os" "github.com/mattn/go-colorable" - "github.com/mattn/go-isatty" "github.com/mgutz/ansi" ) @@ -23,22 +22,12 @@ var ( Bold = makeColorFunc("default+b") ) -func isStdoutTerminal() bool { - if !checkedTerminal { - _isStdoutTerminal = IsTerminal(os.Stdout) - checkedTerminal = true - } - return _isStdoutTerminal -} - -// IsTerminal reports whether the file descriptor is connected to a terminal -func IsTerminal(f *os.File) bool { - return isatty.IsTerminal(f.Fd()) || isatty.IsCygwinTerminal(f.Fd()) -} - // NewColorable returns an output stream that handles ANSI color sequences on Windows -func NewColorable(f *os.File) io.Writer { - return colorable.NewColorable(f) +func NewColorable(w io.Writer) io.Writer { + if f, isFile := w.(*os.File); isFile { + return colorable.NewColorable(f) + } + return w } func makeColorFunc(color string) func(string) string { diff --git a/utils/table_printer.go b/utils/table_printer.go index 217c97ef6..823a4b533 100644 --- a/utils/table_printer.go +++ b/utils/table_printer.go @@ -9,8 +9,6 @@ import ( "strings" "github.com/cli/cli/pkg/text" - "github.com/mattn/go-isatty" - "golang.org/x/crypto/ssh/terminal" ) type TablePrinter interface { @@ -21,26 +19,23 @@ type TablePrinter interface { } func NewTablePrinter(w io.Writer) TablePrinter { - if outFile, isFile := w.(*os.File); isFile { - // TODO: use utils.IsTerminal() - isCygwin := isatty.IsCygwinTerminal(outFile.Fd()) - if isatty.IsTerminal(outFile.Fd()) || isCygwin { - ttyWidth := 80 - if w, _, err := terminal.GetSize(int(outFile.Fd())); err == nil { - ttyWidth = w - } else if isCygwin { - tputCmd := exec.Command("tput", "cols") - tputCmd.Stdin = os.Stdin - if out, err := tputCmd.Output(); err == nil { - if w, err := strconv.Atoi(strings.TrimSpace(string(out))); err == nil { - ttyWidth = w - } + if IsTerminal(w) { + isCygwin := IsCygwinTerminal(w) + ttyWidth := 80 + if termWidth, _, err := TerminalSize(w); err == nil { + ttyWidth = termWidth + } else if isCygwin { + tputCmd := exec.Command("tput", "cols") + tputCmd.Stdin = os.Stdin + if out, err := tputCmd.Output(); err == nil { + if w, err := strconv.Atoi(strings.TrimSpace(string(out))); err == nil { + ttyWidth = w } } - return &ttyTablePrinter{ - out: NewColorable(outFile), - maxWidth: ttyWidth, - } + } + return &ttyTablePrinter{ + out: NewColorable(w), + maxWidth: ttyWidth, } } return &tsvTablePrinter{ diff --git a/utils/terminal.go b/utils/terminal.go new file mode 100644 index 000000000..29d36a184 --- /dev/null +++ b/utils/terminal.go @@ -0,0 +1,44 @@ +package utils + +import ( + "fmt" + "os" + + "github.com/mattn/go-isatty" + "golang.org/x/crypto/ssh/terminal" +) + +func isStdoutTerminal() bool { + if !checkedTerminal { + _isStdoutTerminal = IsTerminal(os.Stdout) + checkedTerminal = true + } + return _isStdoutTerminal +} + +// TODO I don't like this use of interface{} but we need to accept both io.Writer and io.Reader +// interfaces. + +var IsTerminal = func(w interface{}) bool { + if f, isFile := w.(*os.File); isFile { + return isatty.IsTerminal(f.Fd()) || IsCygwinTerminal(f) + } + + return false +} + +func IsCygwinTerminal(w interface{}) bool { + if f, isFile := w.(*os.File); isFile { + return isatty.IsCygwinTerminal(f.Fd()) + } + + return false +} + +var TerminalSize = func(w interface{}) (int, int, error) { + if f, isFile := w.(*os.File); isFile { + return terminal.GetSize(int(f.Fd())) + } + + return 0, 0, fmt.Errorf("%v is not a file", w) +} From 97107661feb955f9481fa3d21423fde1ba353658 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 14 Jul 2020 12:05:59 -0500 Subject: [PATCH 2/3] review feedback --- command/issue.go | 8 ++++---- command/issue_test.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/command/issue.go b/command/issue.go index 720d81669..6a1f6db3a 100644 --- a/command/issue.go +++ b/command/issue.go @@ -315,8 +315,8 @@ func printRawIssuePreview(out io.Writer, issue *api.Issue) error { labels := issueLabelList(*issue) projects := issueProjectList(*issue) - // Print empty strings for empty values so the number of metadata lines is always consistent (ie - // so head -n8 can be relied on) + // Print empty strings for empty values so the number of metadata lines is consistent when + // processing many issues with head and grep. fmt.Fprintf(out, "title:\t%s\n", issue.Title) fmt.Fprintf(out, "state:\t%s\n", issue.State) fmt.Fprintf(out, "author:\t%s\n", issue.Author.Login) @@ -490,7 +490,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) if interactive && !connectedToTerminal(cmd) { - return fmt.Errorf("can't run non-interactively without both --title and --body") + return fmt.Errorf("must provide --title and --body when not attached to a terminal") } if interactive { @@ -662,7 +662,7 @@ func printIssues(w io.Writer, prefix string, totalCount int, issues []api.Issue) if table.IsTTY() { table.AddField(utils.FuzzyAgo(ago), nil, utils.Gray) } else { - table.AddField(fmt.Sprintf("%d", int(ago.Minutes())), nil, nil) + table.AddField(issue.UpdatedAt.String(), nil, nil) } table.EndRow() } diff --git a/command/issue_test.go b/command/issue_test.go index 7ed580c90..68f979171 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -568,7 +568,7 @@ func TestIssueCreate_nontty_error(t *testing.T) { t.Fatal("expected error running command `issue create`") } - assert.Equal(t, "can't run non-interactively without both --title and --body", err.Error()) + assert.Equal(t, "must provide --title and --body when not attached to a terminal", err.Error()) } From 754dcb744caa892a8566ebd6d368a6d60a2e9b09 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 14 Jul 2020 12:36:54 -0500 Subject: [PATCH 3/3] catch up to trunk --- command/issue_test.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/command/issue_test.go b/command/issue_test.go index 68f979171..0b05c26a0 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -114,9 +114,9 @@ func TestIssueList_nontty(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - jsonFile, _ := os.Open("../test/fixtures/issueList.json") - defer jsonFile.Close() - http.StubResponse(200, jsonFile) + http.Register( + httpmock.GraphQL(`query IssueList\b`), + httpmock.FileResponse("../test/fixtures/issueList.json")) output, err := RunCommand("issue list") if err != nil { @@ -381,9 +381,7 @@ func TestIssueView_nontty_Preview(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - jsonFile, _ := os.Open(tc.fixture) - defer jsonFile.Close() - http.StubResponse(200, jsonFile) + http.Register(httpmock.GraphQL(`query IssueByNumber\b`), httpmock.FileResponse(tc.fixture)) output, err := RunCommand(tc.command) if err != nil {