diff --git a/command/issue.go b/command/issue.go index 67c315a9d..6a1f6db3a 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 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) + 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("must provide --title and --body when not attached to a terminal") + } + 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(issue.UpdatedAt.String(), nil, nil) + } table.EndRow() } _ = table.Render() diff --git a/command/issue_test.go b/command/issue_test.go index a3ad56b3a..0b05c26a0 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") + + http.Register( + httpmock.GraphQL(`query IssueList\b`), + httpmock.FileResponse("../test/fixtures/issueList.json")) + + 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,90 @@ 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") + + http.Register(httpmock.GraphQL(`query IssueByNumber\b`), httpmock.FileResponse(tc.fixture)) + + 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 +548,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, "must provide --title and --body when not attached to a terminal", 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) +}