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 5 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
21 changes: 14 additions & 7 deletions internal/output/plain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ func sprintMap(value interface{}) (string, error) {

sort.Slice(result, func(i, j int) bool { return result[i] < result[j] })

return "\n" + strings.Join(result, "\n"), nil
return strings.Join(result, "\n"), nil
MDrakos marked this conversation as resolved.
Show resolved Hide resolved
}

// sprintTable will marshal and return the given slice of structs as a string, formatted as a table
Expand Down Expand Up @@ -299,14 +299,14 @@ func sprintTable(vertical bool, slice []interface{}) (string, error) {
return "", err
}

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

if firstIteration && !funk.Contains(field.opts, string(SeparateLineOpt)) {
headers = append(headers, localizedField(field.l10n))
}

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

Choose a reason for hiding this comment

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

This feels inappropriate. OmitEmpty means "don't print if it's empty". This is doing the exact opposite.

The old implementation is definitely awkward, because if we don't omit the column for each row you're going to run into some weird results. But then further down we also have shiftColsVal which allows for columns to span based on the shiftCols= param. Point is, this may all be by design, and this change you're making is backward incompatible. So either we need to audit existing use of this code and ensure it is compatible with your change, or we don't make any backward incompatible changes.

Copy link
Member

Choose a reason for hiding this comment

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

Note this is for the horizontal table mechanic, I suspect you can just revert this back to what it looked like before this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, that's leftover from my initial implementation. It's been reverted now.


if funk.Contains(field.opts, string(EmptyNil)) && stringValue == nilText {
stringValue = ""
}
Expand All @@ -325,7 +325,8 @@ func sprintTable(vertical bool, slice []interface{}) (string, error) {
row = append(row, columns(offset, stringValue)...)
}

if len(row) > 0 {
// Append row if vertical so we can align headers later
if len(row) > 0 || vertical {
MDrakos marked this conversation as resolved.
Show resolved Hide resolved
rows = append(rows, row)
}
}
Expand All @@ -334,6 +335,7 @@ func sprintTable(vertical bool, slice []interface{}) (string, error) {
t := table.New([]string{"", ""})
t.AddRow(verticalRows(headers, rows)...)
t.HideHeaders = true
t.Vertical = true
return t.Render(), nil
}

Expand Down Expand Up @@ -406,7 +408,12 @@ func verticalRows(hdrs []string, rows [][]string) [][]string {
for j, hcol := range hrow {
var header string
if j < len(hdrs) {
header = hdrs[j]
// Align headers with rows
if hcol == "" {
MDrakos marked this conversation as resolved.
Show resolved Hide resolved
continue
} else {
header = hdrs[j]
}
}

vrow := []string{header, hcol}
Expand Down
2 changes: 1 addition & 1 deletion internal/output/plain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func TestPlain_Print(t *testing.T) {
"int": int(1),
"float": float32(9.1),
}},
"\n float: 9.10 \n int: 1 \n string: hello \n",
" float: 9.10 \n int: 1 \n string: hello \n",
"",
},
{
Expand Down
19 changes: 17 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,11 @@ func (t *Table) calculateWidth(maxTableWidth int) ([]int, int) {
colWidthsCombined += colWidths[n]
}

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 +121,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 +144,27 @@ 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
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
colWidths[len(colWidths)-1] -= diff
MDrakos marked this conversation as resolved.
Show resolved Hide resolved
}
}

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