From 4be04aded367e14539e8b628859a35c852eff59a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 Nov 2019 15:43:28 +0100 Subject: [PATCH 1/5] Fix ANSI color output on Windows --- utils/table_printer.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/utils/table_printer.go b/utils/table_printer.go index 1d763b8ce..451b546e7 100644 --- a/utils/table_printer.go +++ b/utils/table_printer.go @@ -5,6 +5,7 @@ import ( "io" "os" + "github.com/mattn/go-colorable" "golang.org/x/crypto/ssh/terminal" ) @@ -24,7 +25,7 @@ func NewTablePrinter(w io.Writer) TablePrinter { ttyWidth = w } return &ttyTablePrinter{ - out: w, + out: colorable.NewColorable(outFile), maxWidth: ttyWidth, } } From 002aac351945aad74c9b767d2ffb9dfe2d0ddb03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 Nov 2019 17:05:49 +0100 Subject: [PATCH 2/5] Remove global `-B, --current-branch` flag Now `pr list --base` has shorthand `-B` --- command/pr.go | 2 +- command/root.go | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/command/pr.go b/command/pr.go index 5b3a4a16a..cabd27949 100644 --- a/command/pr.go +++ b/command/pr.go @@ -22,7 +22,7 @@ func init() { prListCmd.Flags().IntP("limit", "L", 30, "maximum number of items to fetch") prListCmd.Flags().StringP("state", "s", "open", "filter by state") - prListCmd.Flags().StringP("base", "b", "", "filter by base branch") + prListCmd.Flags().StringP("base", "B", "", "filter by base branch") prListCmd.Flags().StringArrayP("label", "l", nil, "filter by label") } diff --git a/command/root.go b/command/root.go index 489cd7f30..96feb1605 100644 --- a/command/root.go +++ b/command/root.go @@ -19,7 +19,6 @@ var BuildDate = "YYYY-MM-DD" func init() { RootCmd.Version = fmt.Sprintf("%s (%s)", Version, BuildDate) RootCmd.PersistentFlags().StringP("repo", "R", "", "current GitHub repository") - RootCmd.PersistentFlags().StringP("current-branch", "B", "", "current git branch") // TODO: // RootCmd.PersistentFlags().BoolP("verbose", "V", false, "enable verbose output") @@ -55,9 +54,7 @@ func contextForCommand(cmd *cobra.Command) context.Context { ctx := initContext() if repo, err := cmd.Flags().GetString("repo"); err == nil && repo != "" { ctx.SetBaseRepo(repo) - } - if branch, err := cmd.Flags().GetString("current-branch"); err == nil && branch != "" { - ctx.SetBranch(branch) + ctx.SetBranch("master") } return ctx } From b8a0754a0329767491df9710314af0fb938dbd87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 Nov 2019 17:08:28 +0100 Subject: [PATCH 3/5] :nail_care: Sentence case for CLI flags --- command/issue.go | 10 +++++----- command/pr.go | 8 ++++---- command/root.go | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/command/issue.go b/command/issue.go index 0c022939f..f8fa7f059 100644 --- a/command/issue.go +++ b/command/issue.go @@ -32,17 +32,17 @@ func init() { "Supply a title. Will prompt for one otherwise.") issueCreateCmd.Flags().StringP("body", "b", "", "Supply a body. Will prompt for one otherwise.") - issueCreateCmd.Flags().BoolP("web", "w", false, "open the web browser to create an issue") + issueCreateCmd.Flags().BoolP("web", "w", false, "Open the web browser to create an issue") issueListCmd := &cobra.Command{ Use: "list", Short: "List and filter issues in this repository", RunE: issueList, } - issueListCmd.Flags().StringP("assignee", "a", "", "filter by assignee") - issueListCmd.Flags().StringSliceP("label", "l", nil, "filter by label") - issueListCmd.Flags().StringP("state", "s", "", "filter by state (open|closed|all)") - issueListCmd.Flags().IntP("limit", "L", 30, "maximum number of issues to fetch") + issueListCmd.Flags().StringP("assignee", "a", "", "Filter by assignee") + issueListCmd.Flags().StringSliceP("label", "l", nil, "Filter by label") + issueListCmd.Flags().StringP("state", "s", "", "Filter by state (open|closed|all)") + issueListCmd.Flags().IntP("limit", "L", 30, "Maximum number of issues to fetch") issueCmd.AddCommand((issueListCmd)) } diff --git a/command/pr.go b/command/pr.go index cabd27949..5d45de54b 100644 --- a/command/pr.go +++ b/command/pr.go @@ -20,10 +20,10 @@ func init() { prCmd.AddCommand(prStatusCmd) prCmd.AddCommand(prViewCmd) - prListCmd.Flags().IntP("limit", "L", 30, "maximum number of items to fetch") - prListCmd.Flags().StringP("state", "s", "open", "filter by state") - prListCmd.Flags().StringP("base", "B", "", "filter by base branch") - prListCmd.Flags().StringArrayP("label", "l", nil, "filter by label") + prListCmd.Flags().IntP("limit", "L", 30, "Maximum number of items to fetch") + prListCmd.Flags().StringP("state", "s", "open", "Filter by state") + prListCmd.Flags().StringP("base", "B", "", "Filter by base branch") + prListCmd.Flags().StringArrayP("label", "l", nil, "Filter by label") } var prCmd = &cobra.Command{ diff --git a/command/root.go b/command/root.go index 96feb1605..5ae77fe1d 100644 --- a/command/root.go +++ b/command/root.go @@ -18,7 +18,7 @@ var BuildDate = "YYYY-MM-DD" func init() { RootCmd.Version = fmt.Sprintf("%s (%s)", Version, BuildDate) - RootCmd.PersistentFlags().StringP("repo", "R", "", "current GitHub repository") + RootCmd.PersistentFlags().StringP("repo", "R", "", "Current GitHub repository") // TODO: // RootCmd.PersistentFlags().BoolP("verbose", "V", false, "enable verbose output") From 854a4b3fdffddad53cfe9d129cae41a5560b39b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 Nov 2019 17:26:27 +0100 Subject: [PATCH 4/5] :nail_care: Sentence-case for `--help` and `--version` flags --- command/root.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/command/root.go b/command/root.go index 5ae77fe1d..0e24de8b5 100644 --- a/command/root.go +++ b/command/root.go @@ -19,6 +19,8 @@ var BuildDate = "YYYY-MM-DD" func init() { RootCmd.Version = fmt.Sprintf("%s (%s)", Version, BuildDate) RootCmd.PersistentFlags().StringP("repo", "R", "", "Current GitHub repository") + RootCmd.PersistentFlags().Bool("help", false, "Show help for command") + RootCmd.Flags().Bool("version", false, "Print gh version") // TODO: // RootCmd.PersistentFlags().BoolP("verbose", "V", false, "enable verbose output") From b6fa88337d6330f246292a89bc7ff26a04845666 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 Nov 2019 20:51:51 +0100 Subject: [PATCH 5/5] Ensure that commands print to a colorable output If a command does `fmt.Print(...)` for output that contains ANSI color codes, this not safe on Windows. We have to ensure that we always use the `fmt.Fprint*` family of functions with a writer that was transformed using `utils.NewColorable()`. --- command/issue.go | 38 ++++++++++++++++++------------- command/pr.go | 51 ++++++++++++++++++++++-------------------- command/root.go | 10 +++++++++ utils/color.go | 10 ++++++++- utils/table_printer.go | 3 +-- 5 files changed, 69 insertions(+), 43 deletions(-) diff --git a/command/issue.go b/command/issue.go index 4fc937a1e..53db25ad3 100644 --- a/command/issue.go +++ b/command/issue.go @@ -2,6 +2,7 @@ package command import ( "fmt" + "io" "os" "strconv" "strings" @@ -94,12 +95,15 @@ func issueList(cmd *cobra.Command, args []string) error { return err } + out := cmd.OutOrStdout() + colorOut := colorableOut(cmd) + if len(issues) == 0 { - printMessage("There are no open issues") + printMessage(colorOut, "There are no open issues") return nil } - table := utils.NewTablePrinter(cmd.OutOrStdout()) + table := utils.NewTablePrinter(out) for _, issue := range issues { issueNum := strconv.Itoa(issue.Number) if table.IsTTY() { @@ -141,30 +145,32 @@ func issueStatus(cmd *cobra.Command, args []string) error { return err } - printHeader("Issues assigned to you") + out := colorableOut(cmd) + + printHeader(out, "Issues assigned to you") if issuePayload.Assigned != nil { - printIssues(" ", issuePayload.Assigned...) + printIssues(out, " ", issuePayload.Assigned...) } else { message := fmt.Sprintf(" There are no issues assgined to you") - printMessage(message) + printMessage(out, message) } - fmt.Println() + fmt.Fprintln(out) - printHeader("Issues mentioning you") + printHeader(out, "Issues mentioning you") if len(issuePayload.Mentioned) > 0 { - printIssues(" ", issuePayload.Mentioned...) + printIssues(out, " ", issuePayload.Mentioned...) } else { - printMessage(" There are no issues mentioning you") + printMessage(out, " There are no issues mentioning you") } - fmt.Println() + fmt.Fprintln(out) - printHeader("Issues opened by you") + printHeader(out, "Issues opened by you") if len(issuePayload.Authored) > 0 { - printIssues(" ", issuePayload.Authored...) + printIssues(out, " ", issuePayload.Authored...) } else { - printMessage(" There are no issues opened by you") + printMessage(out, " There are no issues opened by you") } - fmt.Println() + fmt.Fprintln(out) return nil } @@ -255,14 +261,14 @@ func issueCreate(cmd *cobra.Command, args []string) error { return nil } -func printIssues(prefix string, issues ...api.Issue) { +func printIssues(w io.Writer, prefix string, issues ...api.Issue) { for _, issue := range issues { number := utils.Green("#" + strconv.Itoa(issue.Number)) coloredLabels := labelList(issue) if coloredLabels != "" { coloredLabels = utils.Gray(fmt.Sprintf(" (%s)", coloredLabels)) } - fmt.Printf("%s%s %s%s\n", prefix, number, truncate(70, issue.Title), coloredLabels) + fmt.Fprintf(w, "%s%s %s%s\n", prefix, number, truncate(70, issue.Title), coloredLabels) } } diff --git a/command/pr.go b/command/pr.go index 5b3a4a16a..3fdf42964 100644 --- a/command/pr.go +++ b/command/pr.go @@ -2,6 +2,7 @@ package command import ( "fmt" + "io" "os" "os/exec" "strconv" @@ -78,30 +79,32 @@ func prStatus(cmd *cobra.Command, args []string) error { return err } - printHeader("Current branch") + out := colorableOut(cmd) + + printHeader(out, "Current branch") if prPayload.CurrentPR != nil { - printPrs(*prPayload.CurrentPR) + printPrs(out, *prPayload.CurrentPR) } else { message := fmt.Sprintf(" There is no pull request associated with %s", utils.Cyan("["+currentBranch+"]")) - printMessage(message) + printMessage(out, message) } - fmt.Println() + fmt.Fprintln(out) - printHeader("Created by you") + printHeader(out, "Created by you") if len(prPayload.ViewerCreated) > 0 { - printPrs(prPayload.ViewerCreated...) + printPrs(out, prPayload.ViewerCreated...) } else { - printMessage(" You have no open pull requests") + printMessage(out, " You have no open pull requests") } - fmt.Println() + fmt.Fprintln(out) - printHeader("Requesting a code review from you") + printHeader(out, "Requesting a code review from you") if len(prPayload.ReviewRequested) > 0 { - printPrs(prPayload.ReviewRequested...) + printPrs(out, prPayload.ReviewRequested...) } else { - printMessage(" You have no pull requests to review") + printMessage(out, " You have no pull requests to review") } - fmt.Println() + fmt.Fprintln(out) return nil } @@ -330,15 +333,15 @@ func prCheckout(cmd *cobra.Command, args []string) error { return nil } -func printPrs(prs ...api.PullRequest) { +func printPrs(w io.Writer, prs ...api.PullRequest) { for _, pr := range prs { prNumber := fmt.Sprintf("#%d", pr.Number) - fmt.Printf(" %s %s %s", utils.Yellow(prNumber), truncate(50, pr.Title), utils.Cyan("["+pr.HeadLabel()+"]")) + fmt.Fprintf(w, " %s %s %s", utils.Yellow(prNumber), truncate(50, pr.Title), utils.Cyan("["+pr.HeadLabel()+"]")) checks := pr.ChecksStatus() reviews := pr.ReviewStatus() if checks.Total > 0 || reviews.ChangesRequested || reviews.Approved { - fmt.Printf("\n ") + fmt.Fprintf(w, "\n ") } if checks.Total > 0 { @@ -354,27 +357,27 @@ func printPrs(prs ...api.PullRequest) { } else if checks.Passing == checks.Total { summary = utils.Green("Checks passing") } - fmt.Printf(" - %s", summary) + fmt.Fprintf(w, " - %s", summary) } if reviews.ChangesRequested { - fmt.Printf(" - %s", utils.Red("changes requested")) + fmt.Fprintf(w, " - %s", utils.Red("changes requested")) } else if reviews.ReviewRequired { - fmt.Printf(" - %s", utils.Yellow("review required")) + fmt.Fprintf(w, " - %s", utils.Yellow("review required")) } else if reviews.Approved { - fmt.Printf(" - %s", utils.Green("approved")) + fmt.Fprintf(w, " - %s", utils.Green("approved")) } - fmt.Printf("\n") + fmt.Fprint(w, "\n") } } -func printHeader(s string) { - fmt.Println(utils.Bold(s)) +func printHeader(w io.Writer, s string) { + fmt.Fprintln(w, utils.Bold(s)) } -func printMessage(s string) { - fmt.Println(utils.Gray(s)) +func printMessage(w io.Writer, s string) { + fmt.Fprintln(w, utils.Gray(s)) } func truncate(maxLength int, title string) string { diff --git a/command/root.go b/command/root.go index e8764c44b..da730dd2b 100644 --- a/command/root.go +++ b/command/root.go @@ -2,11 +2,13 @@ package command import ( "fmt" + "io" "os" "strings" "github.com/github/gh-cli/api" "github.com/github/gh-cli/context" + "github.com/github/gh-cli/utils" "github.com/spf13/cobra" ) @@ -93,3 +95,11 @@ var apiClientForContext = func(ctx context.Context) (*api.Client, error) { } return api.NewClient(opts...), nil } + +func colorableOut(cmd *cobra.Command) io.Writer { + out := cmd.OutOrStdout() + if outFile, isFile := out.(*os.File); isFile { + return utils.NewColorable(outFile) + } + return out +} diff --git a/utils/color.go b/utils/color.go index fb8479734..b955c4755 100644 --- a/utils/color.go +++ b/utils/color.go @@ -1,11 +1,19 @@ package utils import ( + "io" + "os" + + "github.com/mattn/go-colorable" "github.com/mattn/go-isatty" "github.com/mgutz/ansi" - "os" ) +// NewColorable returns an output stream that handles ANSI color sequences on Windows +func NewColorable(f *os.File) io.Writer { + return colorable.NewColorable(f) +} + func makeColorFunc(color string) func(string) string { return func(arg string) string { output := arg diff --git a/utils/table_printer.go b/utils/table_printer.go index 89b1aebce..649e246e1 100644 --- a/utils/table_printer.go +++ b/utils/table_printer.go @@ -8,7 +8,6 @@ import ( "strconv" "strings" - "github.com/mattn/go-colorable" "github.com/mattn/go-isatty" "golang.org/x/crypto/ssh/terminal" ) @@ -37,7 +36,7 @@ func NewTablePrinter(w io.Writer) TablePrinter { } } return &ttyTablePrinter{ - out: colorable.NewColorable(outFile), + out: NewColorable(outFile), maxWidth: ttyWidth, } }