From d727f0b8c080095310a5c9fa86a0878ed1b98b5c Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Thu, 19 Oct 2023 17:10:20 +0200 Subject: [PATCH] Some polish and style nits --- .golangci.yml | 5 -- internal/tableprinter/table_printer.go | 24 ++-------- internal/tableprinter/table_printer_test.go | 47 ------------------- internal/text/text.go | 4 ++ pkg/cmd/gist/list/list.go | 1 - pkg/cmd/issue/shared/display.go | 2 +- pkg/cmd/label/list.go | 2 +- pkg/cmd/pr/list/list.go | 2 +- pkg/cmd/project/field-list/field_list_test.go | 4 ++ pkg/cmd/project/item-list/item_list_test.go | 3 ++ pkg/cmd/project/list/list_test.go | 6 +++ pkg/cmd/release/view/view.go | 4 +- pkg/cmd/repo/list/list.go | 2 +- pkg/cmd/status/status.go | 4 +- pkg/iostreams/color.go | 37 +++++++-------- 15 files changed, 48 insertions(+), 99 deletions(-) delete mode 100644 internal/tableprinter/table_printer_test.go diff --git a/.golangci.yml b/.golangci.yml index c70fac5437f..ff7f3701405 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -6,8 +6,3 @@ linters: issues: max-issues-per-linter: 0 max-same-issues: 0 - exclude-rules: - - path: _test\.go - linters: - - staticcheck - text: "SA1019: tableprinter.NoHeader is deprecated: use WithHeader unless required otherwise." diff --git a/internal/tableprinter/table_printer.go b/internal/tableprinter/table_printer.go index 5b5cab4cbbd..8a48edc689e 100644 --- a/internal/tableprinter/table_printer.go +++ b/internal/tableprinter/table_printer.go @@ -64,26 +64,21 @@ func NewWithWriter(w io.Writer, isTTY bool, maxWidth int, cs *iostreams.ColorSch if isTTY && len(headers.columns) > 0 { // Make sure all headers are uppercase. for i := range headers.columns { - // TODO: Consider truncating longer headers e.g., NUMBER, or removing unnecessary headers e.g., DESCRIPTION with no descriptions. headers.columns[i] = strings.ToUpper(headers.columns[i]) } - // Make sure all header columns are padded - even the last one - to apply the proper TableHeader style. - // Checking cs.Enabled() avoids having to do that for nearly all CLI tests. + // Make sure all header columns are padded - even the last one. Previously, the last header column + // was not padded. In tests cs.Enabled() is false which allows us to avoid having to fix up + // numerous tests that verify header padding. var paddingFunc func(int, string) string if cs.Enabled() { - paddingFunc = func(width int, text string) string { - if l := len(text); l < width { - return text + strings.Repeat(" ", width-l) - } - return text - } + paddingFunc = text.PadRight } tp.AddHeader( headers.columns, tableprinter.WithPadding(paddingFunc), - tableprinter.WithColor(cs.TableHeader), + tableprinter.WithColor(cs.GrayUnderline), ) } @@ -103,12 +98,3 @@ func WithHeader(columns ...string) headerOption { // // Deprecated: use WithHeader unless required otherwise. var NoHeader = headerOption{} - -// TruncateNonURL truncates any text that does not begin with "https://" or "http://". -// This is provided for backward compatibility with the old table printer for existing tables. -func TruncateNonURL(maxWidth int, s string) string { - if strings.HasPrefix(s, "https://") || strings.HasPrefix(s, "http://") { - return s - } - return text.Truncate(maxWidth, s) -} diff --git a/internal/tableprinter/table_printer_test.go b/internal/tableprinter/table_printer_test.go deleted file mode 100644 index 77ebb23fdd1..00000000000 --- a/internal/tableprinter/table_printer_test.go +++ /dev/null @@ -1,47 +0,0 @@ -package tableprinter - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestTruncateNonURL(t *testing.T) { - tests := []struct { - name string - input string - want string - }{ - { - name: "http url", - input: "http://github.com", - want: "http://github.com", - }, - { - name: "https url", - input: "https://github.com", - want: "https://github.com", - }, - { - name: "https url", - input: "https://github.com", - want: "https://github.com", - }, - { - name: "git url", - input: "git@github.com", - want: "git@git...", - }, - { - name: "non-url", - input: "just some plain text", - want: "just so...", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := TruncateNonURL(10, tt.input) - assert.Equal(t, tt.want, got) - }) - } -} diff --git a/internal/text/text.go b/internal/text/text.go index 6eb54158481..9ecfad93b50 100644 --- a/internal/text/text.go +++ b/internal/text/text.go @@ -77,3 +77,7 @@ func DisplayURL(urlStr string) string { func RemoveDiacritics(value string) string { return text.RemoveDiacritics(value) } + +func PadRight(maxWidth int, s string) string { + return text.PadRight(maxWidth, s) +} diff --git a/pkg/cmd/gist/list/list.go b/pkg/cmd/gist/list/list.go index a73d4a26ff2..ee9fb25396d 100644 --- a/pkg/cmd/gist/list/list.go +++ b/pkg/cmd/gist/list/list.go @@ -120,7 +120,6 @@ func listRun(opts *ListOptions) error { tp.AddField( text.RemoveExcessiveWhitespace(description), tableprinter.WithColor(cs.Bold), - tableprinter.WithTruncate(tableprinter.TruncateNonURL), ) tp.AddField(text.Pluralize(fileCount, "file")) tp.AddField(visibility, tableprinter.WithColor(visColor)) diff --git a/pkg/cmd/issue/shared/display.go b/pkg/cmd/issue/shared/display.go index 79efe501992..0c56ffd2cae 100644 --- a/pkg/cmd/issue/shared/display.go +++ b/pkg/cmd/issue/shared/display.go @@ -36,7 +36,7 @@ func PrintIssues(io *iostreams.IOStreams, now time.Time, prefix string, totalCou if !isTTY { table.AddField(issue.State) } - table.AddField(text.RemoveExcessiveWhitespace(issue.Title), tableprinter.WithTruncate(tableprinter.TruncateNonURL)) + table.AddField(text.RemoveExcessiveWhitespace(issue.Title)) table.AddField(issueLabelList(&issue, cs, isTTY)) table.AddTimeField(now, issue.UpdatedAt, cs.Gray) table.EndRow() diff --git a/pkg/cmd/label/list.go b/pkg/cmd/label/list.go index 4e3f7fa0825..7b4c95ed1db 100644 --- a/pkg/cmd/label/list.go +++ b/pkg/cmd/label/list.go @@ -138,7 +138,7 @@ func printLabels(io *iostreams.IOStreams, labels []label) error { for _, label := range labels { table.AddField(label.Name, tableprinter.WithColor(cs.ColorFromRGB(label.Color))) - table.AddField(label.Description, tableprinter.WithTruncate(tableprinter.TruncateNonURL)) + table.AddField(label.Description) table.AddField("#" + label.Color) table.EndRow() diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index fcec196ebbe..e900a5d7bd7 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -219,7 +219,7 @@ func listRun(opts *ListOptions) error { } table.AddField(prNum, tableprinter.WithColor(cs.ColorFromString(shared.ColorForPRState(pr)))) - table.AddField(text.RemoveExcessiveWhitespace(pr.Title), tableprinter.WithTruncate(tableprinter.TruncateNonURL)) + table.AddField(text.RemoveExcessiveWhitespace(pr.Title)) table.AddField(pr.HeadLabel(), tableprinter.WithColor(cs.Cyan)) if !isTTY { table.AddField(prStateWithDraft(&pr)) diff --git a/pkg/cmd/project/field-list/field_list_test.go b/pkg/cmd/project/field-list/field_list_test.go index 70f7ff19ff0..85e67451f50 100644 --- a/pkg/cmd/project/field-list/field_list_test.go +++ b/pkg/cmd/project/field-list/field_list_test.go @@ -163,6 +163,7 @@ func TestRunList_User(t *testing.T) { ios, _, stdout, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{ number: 1, @@ -256,6 +257,7 @@ func TestRunList_Org(t *testing.T) { ios, _, stdout, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{ number: 1, @@ -339,6 +341,7 @@ func TestRunList_Me(t *testing.T) { ios, _, stdout, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{ number: 1, @@ -406,6 +409,7 @@ func TestRunList_Empty(t *testing.T) { ios, _, _, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{ number: 1, diff --git a/pkg/cmd/project/item-list/item_list_test.go b/pkg/cmd/project/item-list/item_list_test.go index 17c23b33c14..c36dfc589c4 100644 --- a/pkg/cmd/project/item-list/item_list_test.go +++ b/pkg/cmd/project/item-list/item_list_test.go @@ -178,6 +178,7 @@ func TestRunList_User(t *testing.T) { ios, _, stdout, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{ number: 1, @@ -285,6 +286,7 @@ func TestRunList_Org(t *testing.T) { ios, _, stdout, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{ number: 1, @@ -382,6 +384,7 @@ func TestRunList_Me(t *testing.T) { ios, _, stdout, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{ number: 1, diff --git a/pkg/cmd/project/list/list_test.go b/pkg/cmd/project/list/list_test.go index 3e77bcdbc14..bafdf8e93d0 100644 --- a/pkg/cmd/project/list/list_test.go +++ b/pkg/cmd/project/list/list_test.go @@ -154,6 +154,7 @@ func TestRunList(t *testing.T) { ios, _, stdout, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{ owner: "monalisa", @@ -225,6 +226,7 @@ func TestRunList_Me(t *testing.T) { ios, _, stdout, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{ owner: "@me", @@ -296,6 +298,7 @@ func TestRunListViewer(t *testing.T) { ios, _, stdout, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{}, client: client, @@ -374,6 +377,7 @@ func TestRunListOrg(t *testing.T) { ios, _, stdout, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{ owner: "github", @@ -428,6 +432,7 @@ func TestRunListEmpty(t *testing.T) { ios, _, _, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{}, client: client, @@ -504,6 +509,7 @@ func TestRunListWithClosed(t *testing.T) { ios, _, stdout, _ := iostreams.Test() config := listConfig{ + //nolint:staticcheck // SA1019: tableprinter.NoHeaders temporarily allowed in project tests. tp: tableprinter.New(ios, tableprinter.NoHeader), opts: listOpts{ owner: "monalisa", diff --git a/pkg/cmd/release/view/view.go b/pkg/cmd/release/view/view.go index 17a82e0c0ad..a32482e65ab 100644 --- a/pkg/cmd/release/view/view.go +++ b/pkg/cmd/release/view/view.go @@ -157,8 +157,8 @@ func renderReleaseTTY(io *iostreams.IOStreams, release *shared.Release) error { //nolint:staticcheck // SA1019: Showing NAME|SIZE headers adds nothing to table. table := tableprinter.New(io, tableprinter.NoHeader) for _, a := range release.Assets { - table.AddField(a.Name, tableprinter.WithTruncate(tableprinter.TruncateNonURL)) - table.AddField(humanFileSize(a.Size), tableprinter.WithTruncate(tableprinter.TruncateNonURL)) + table.AddField(a.Name) + table.AddField(humanFileSize(a.Size)) table.EndRow() } err := table.Render() diff --git a/pkg/cmd/repo/list/list.go b/pkg/cmd/repo/list/list.go index 52313424263..f91b847a73c 100644 --- a/pkg/cmd/repo/list/list.go +++ b/pkg/cmd/repo/list/list.go @@ -193,7 +193,7 @@ func listRun(opts *ListOptions) error { } tp.AddField(repo.NameWithOwner, tableprinter.WithColor(cs.Bold)) - tp.AddField(text.RemoveExcessiveWhitespace(repo.Description), tableprinter.WithTruncate(tableprinter.TruncateNonURL)) + tp.AddField(text.RemoveExcessiveWhitespace(repo.Description)) tp.AddField(info, tableprinter.WithColor(infoColor)) if tp.IsTTY() { tp.AddField(text.FuzzyAgoAbbr(opts.Now(), *t), tableprinter.WithColor(cs.Gray)) diff --git a/pkg/cmd/status/status.go b/pkg/cmd/status/status.go index d1fd214d917..0ed7dd3a7fc 100644 --- a/pkg/cmd/status/status.go +++ b/pkg/cmd/status/status.go @@ -688,9 +688,9 @@ func statusRun(opts *StatusOptions) error { } tp.AddField(si.Identifier, tableprinter.WithColor(idStyle), tableprinter.WithTruncate(nil)) if si.Reason != "" { - tp.AddField(si.Reason, tableprinter.WithTruncate(tableprinter.TruncateNonURL)) + tp.AddField(si.Reason) } - tp.AddField(si.Preview(), tableprinter.WithTruncate(tableprinter.TruncateNonURL)) + tp.AddField(si.Preview()) tp.EndRow() } diff --git a/pkg/iostreams/color.go b/pkg/iostreams/color.go index 6652c60d13e..fd69b0273ae 100644 --- a/pkg/iostreams/color.go +++ b/pkg/iostreams/color.go @@ -9,22 +9,21 @@ import ( ) var ( - magenta = ansi.ColorFunc("magenta") - cyan = ansi.ColorFunc("cyan") - red = ansi.ColorFunc("red") - yellow = ansi.ColorFunc("yellow") - blue = ansi.ColorFunc("blue") - green = ansi.ColorFunc("green") - gray = ansi.ColorFunc("black+h") - bold = ansi.ColorFunc("default+b") - cyanBold = ansi.ColorFunc("cyan+b") - greenBold = ansi.ColorFunc("green+b") + magenta = ansi.ColorFunc("magenta") + cyan = ansi.ColorFunc("cyan") + red = ansi.ColorFunc("red") + yellow = ansi.ColorFunc("yellow") + blue = ansi.ColorFunc("blue") + green = ansi.ColorFunc("green") + gray = ansi.ColorFunc("black+h") + grayUnderline = ansi.ColorFunc("white+du") + bold = ansi.ColorFunc("default+b") + cyanBold = ansi.ColorFunc("cyan+b") + greenBold = ansi.ColorFunc("green+b") gray256 = func(t string) string { return fmt.Sprintf("\x1b[%d;5;%dm%s\x1b[m", 38, 242, t) } - - tableHeader = ansi.ColorFunc("white+du") ) func NewColorScheme(enabled, is256enabled bool, trueColor bool) *ColorScheme { @@ -110,6 +109,13 @@ func (c *ColorScheme) Grayf(t string, args ...interface{}) string { return c.Gray(fmt.Sprintf(t, args...)) } +func (c *ColorScheme) GrayUnderline(t string) string { + if !c.enabled { + return t + } + return grayUnderline(t) +} + func (c *ColorScheme) Magenta(t string) string { if !c.enabled { return t @@ -170,13 +176,6 @@ func (c *ColorScheme) FailureIconWithColor(colo func(string) string) string { return colo("X") } -func (c *ColorScheme) TableHeader(t string) string { - if !c.enabled { - return t - } - return tableHeader(t) -} - func (c *ColorScheme) ColorFromString(s string) func(string) string { s = strings.ToLower(s) var fn func(string) string