Fix table spacing and omitEmpty option
#3139
Conversation
github-actions
bot
changed the base branch from
master
to
version/0-44-0-RC1
|
I expect we may want to iterate on this but I wanted to try a different approach where we bind the header to the row content. This fixes the issue where the headers and rows could get misaligned. The new function for printing the vertical table does have some repeated logic but I think this starts to take us in the direction we ultimately want to go where we have a separate path for vertical tables rather than trying to have our existing tables with a vertical option. |
internal/output/plain.go
Outdated
| if funk.Contains(field.opts, string(OmitEmpty)) && (stringValue == "" || stringValue == nilText) { | ||
| stringValue = "" | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Note this is for the horizontal table mechanic, I suspect you can just revert this back to what it looked like before this PR?
There was a problem hiding this comment.
Whoops, that's leftover from my initial implementation. It's been reverted now.
| var verticalHeaderWidth int | ||
| if len(colWidths) > 0 && t.Vertical { | ||
| verticalHeaderWidth = colWidths[0] | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
As we discussed in stand, it might be worthwhile to have a separate vertical table representation rather than trying to have a vertical toggle in the standard table representation. We could do this within the current library or look into the
charmfamily of libraries to see if we can get something 3rd party to work.