Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix table spacing and omitEmpty option #3139

Merged
merged 15 commits into from
Mar 20, 2024
Merged

Fix table spacing and omitEmpty option #3139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f46564e
Fix omitEmpty and align headers
MDrakos Feb 23, 2024
72f0998
Fix vertical header spacing
MDrakos Feb 23, 2024
fdeda2c
Fix unit test
MDrakos Feb 23, 2024
f4d6eab
Fix more tests
MDrakos Feb 24, 2024
5c2b63a
Merge branch 'version/0-44-0-RC1' into DX-2578
MDrakos Mar 1, 2024
449463d
Merge branch 'version/0-44-0-RC1' into DX-2578
MDrakos Mar 13, 2024
1cc0a89
Use separate vertical table printing function
MDrakos Mar 13, 2024
b6472f9
Fix test
MDrakos Mar 13, 2024
118a135
Apply diff over remaining columns
MDrakos Mar 18, 2024
402b685
Renaming for clarity
MDrakos Mar 18, 2024
2326e0d
Revert some changes made for initial implementation
MDrakos Mar 19, 2024
dc58a4c
Add comment and fix equalization
MDrakos Mar 19, 2024
8393fe3
Revert some sprintTable changes
MDrakos Mar 19, 2024
d3bd42d
Add comment
MDrakos Mar 19, 2024
b07d536
Don't initialize separate header variable
MDrakos Mar 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 57 additions & 14 deletions internal/output/plain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,10 @@ func sprintTable(vertical bool, slice []interface{}) (string, error) {
return "", nil
}

if vertical {
return sprintVerticalTable(slice)
}

headers := []string{}
rows := [][]string{}
for _, v := range slice {
Expand Down Expand Up @@ -330,14 +334,58 @@ func sprintTable(vertical bool, slice []interface{}) (string, error) {
}
}

if vertical {
t := table.New([]string{"", ""})
t.AddRow(verticalRows(headers, rows)...)
t.HideHeaders = true
return t.Render(), nil
return table.New(headers).AddRow(rows...).Render(), nil
}

type verticalRow struct {
header string
content string
}

func sprintVerticalTable(slice []interface{}) (string, error) {
if len(slice) == 0 {
return "", nil
}

return table.New(headers).AddRow(rows...).Render(), nil
rows := [][]verticalRow{}
for _, v := range slice {
meta, err := parseStructMeta(v)
if err != nil {
return "", err
}

row := []verticalRow{}
for _, field := range meta {
if funk.Contains(field.opts, string(HidePlain)) {
continue
}

stringValue, err := sprint(field.value)
if err != nil {
return "", err
}

if funk.Contains(field.opts, string(OmitEmpty)) && (stringValue == "" || stringValue == nilText) {
continue
}

if funk.Contains(field.opts, string(EmptyNil)) && stringValue == nilText {
stringValue = ""
}

row = append(row, verticalRow{header: localizedField(field.l10n), content: stringValue})
}

if len(row) > 0 {
rows = append(rows, row)
}
}

t := table.New([]string{"", ""})
t.AddRow(verticalRows(rows)...)
t.HideHeaders = true
t.Vertical = true
return t.Render(), nil
}

func asSlice(val interface{}) ([]interface{}, error) {
Expand Down Expand Up @@ -399,17 +447,12 @@ func columns(offset int, value string) []string {
return cols
}

func verticalRows(hdrs []string, rows [][]string) [][]string {
func verticalRows(rows [][]verticalRow) [][]string {
var vrows [][]string

for i, hrow := range rows {
for j, hcol := range hrow {
var header string
if j < len(hdrs) {
header = hdrs[j]
}

vrow := []string{header, hcol}
for _, hcol := range hrow {
vrow := []string{hcol.header, hcol.content}
vrows = append(vrows, vrow)
}

Expand Down
25 changes: 23 additions & 2 deletions internal/table/table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type Table struct {
rows []row

HideHeaders bool
Vertical bool
}

func New(headers []string) *Table {
Expand Down Expand Up @@ -103,6 +104,13 @@ func (t *Table) calculateWidth(maxTableWidth int) ([]int, int) {
colWidthsCombined += colWidths[n]
}

// Capture the width of the vertical header before we equalize the column widths.
// We must respect this width when rescaling the columns.
var verticalHeaderWidth int
if len(colWidths) > 0 && t.Vertical {
verticalHeaderWidth = colWidths[0]
}
Comment on lines +109 to +112
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used on rescaleColumns and is based on inputs that are also passed to that function. So I suggest moving this logic inside rescaleColumns.

But also; what is the rationale here? Comment explaining this would be good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment, but left this here. There is a call further down to an equalizeWidths function that changes the column widths. Before that call the value of the first column is the original header width, which we want to respect when rescaling. I think it makes sense to preserve this, equalize all of the columns, and then ensure the header width is respected when rescaling, but I'm open to move this somewhere else or take a different approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I think you missed the first part of my comment though:

This is only used on rescaleColumns and is based on inputs that are also passed to that function. So I suggest moving this logic inside rescaleColumns.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I didn't explain well exactly what's happening here.

The call to equalizeWidths changes the actual values in the colWidths slice. So we have to grab the column width of the first entry in the slice at this point as this is the max width of the headers column. So verticalHeaderWidth can be a different value from colWidths[0] after the equalizeWidths call.


if colWidthsCombined >= maxTableWidth {
// Equalize widths by 20% of average width.
// This is to prevent columns that are much larger than others
Expand All @@ -115,7 +123,7 @@ func (t *Table) calculateWidth(maxTableWidth int) ([]int, int) {
tableWidth = mathutils.MinInt(tableWidth, maxTableWidth)

// Now scale back the row sizes according to the max width
rescaleColumns(colWidths, tableWidth)
rescaleColumns(colWidths, tableWidth, t.Vertical, verticalHeaderWidth)
logging.Debug("Table column widths: %v, total: %d", colWidths, tableWidth)

return colWidths, tableWidth
Expand All @@ -138,18 +146,31 @@ func equalizeWidths(colWidths []int, percentage int) {
}
}

func rescaleColumns(colWidths []int, targetTotal int) {
func rescaleColumns(colWidths []int, targetTotal int, vertical bool, verticalHeaderWidth int) {
total := float64(mathutils.Total(colWidths...))
multiplier := float64(targetTotal) / total

originalWidths := make([]int, len(colWidths))
for n := range colWidths {
originalWidths[n] = colWidths[n]
colWidths[n] = int(float64(colWidths[n]) * multiplier)
}

// Account for floats that got rounded
if len(colWidths) > 0 {
colWidths[len(colWidths)-1] += targetTotal - mathutils.Total(colWidths...)
}

// If vertical, respect the header width
// verticalHeaderWidth is the width of the header column before we equalized the column widths.
// We compare the current width of the header column with the original width and adjust the other columns accordingly.
if vertical && len(colWidths) > 0 && colWidths[0] < verticalHeaderWidth {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like verticalHeaderWidth is either the value of colWidths[0] or just 0. So either this conditional doesn't trigger, or it triggers if colWidths[0] < 0, which feels inappropriate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in my other comment, verticalHeaderWidth is the value before equalization so it can differ from colWidths[0]. I'll add a comment here as well.

diff := verticalHeaderWidth - colWidths[0]
colWidths[0] += diff
for i := 1; i < len(colWidths); i++ {
colWidths[i] -= diff / (len(colWidths) - 1)
}
}
}

func renderRow(providedColumns []string, colWidths []int) string {
Expand Down
9 changes: 8 additions & 1 deletion internal/table/table_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func TestTable_colWidths(t1 *testing.T) {
{[]string{"1", "2", "3"}},
},
false,
false,
},
100,
},
Expand All @@ -42,6 +43,7 @@ func TestTable_colWidths(t1 *testing.T) {
{[]string{"1", "2", "3"}},
},
false,
false,
},
100,
},
Expand All @@ -57,6 +59,7 @@ func TestTable_colWidths(t1 *testing.T) {
{[]string{"1", "0123456789012345678901234567890123456789"}},
},
false,
false,
},
100,
},
Expand All @@ -71,6 +74,7 @@ func TestTable_colWidths(t1 *testing.T) {
{[]string{"123", "1234", "12345"}},
},
false,
false,
},
100,
},
Expand All @@ -85,6 +89,7 @@ func TestTable_colWidths(t1 *testing.T) {
{[]string{"1", "2", "3"}},
},
false,
false,
},
100,
},
Expand All @@ -100,6 +105,7 @@ func TestTable_colWidths(t1 *testing.T) {
{[]string{strings.Repeat(" ", 100)}},
},
false,
false,
},
100,
},
Expand All @@ -115,6 +121,7 @@ func TestTable_colWidths(t1 *testing.T) {
{[]string{"1", strings.Repeat(" ", 200)}},
},
false,
false,
},
100,
},
Expand Down Expand Up @@ -333,7 +340,7 @@ func Test_rescaleColumns(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
rescaleColumns(tt.args.colWidths, tt.args.targetTotal)
rescaleColumns(tt.args.colWidths, tt.args.targetTotal, false, tt.args.colWidths[0])
if !reflect.DeepEqual(tt.args.colWidths, tt.want) {
t.Errorf("rescaleColumns() got = %v, want %v", tt.args.colWidths, tt.want)
}
Expand Down
Loading