table: fix rebalancing of long merged columns, fixes #350 #353
Conversation
Signed-off-by: Kevin Conner <kev.conner@gmail.com>
|
There was a problem hiding this comment.
I appreciate the compliments. Started out as my first "learn-golang" project, and I needed a table writer library with more control over the styles and sorting. 👍🏽
As to this PR, this is awesome. Thanks for fixing this bug. Before I merge it, can you take a look to see if the one comment below is a genuine bug and if it can be fixed?
| │ 2 │ a.a.a.a │ Pod 1A │ NS 1A │ C 2 │ Y │ | ||
| ├───┼──────────┼──────────┼──────────┼──────────┼─────────────────────────────────────────────────────────┤ | ||
| │ 3 │ a.a.a.a │ Pod 1A │ NS 1A │ C 2 │ Y │ | ||
| └───┴──────────┴──────────┴──────────┴──────────┴─────────────────────────────────────────────────────────┘`) |
There was a problem hiding this comment.
It looks like there might be a tiny bug with centering, or measuring the max length. In Row 1, merged Column 5/6 and merged Column 7/8 contents can be shrunk down further by 2 spaces (1 on each side).
There was a problem hiding this comment.
I can take a look at the details later this evening, will let you know what I find out.
There was a problem hiding this comment.
Thanks! Not a deal breaker, but we may as well try while this code is fresh in our minds.
There was a problem hiding this comment.
It's a slight difference in implementation, the previous code was rounding down the mergedColumnLength when dividing by the numMergedColumns
go-pretty/table/render_init.go
Line 82 in bfe1b7c
In this instance mergedColumnLength was 23, numMergedColums 2 and maxLengthSplitAcrossColumns was therefore 11 (22 instead of 23, missing one space).
The new code keeps track of the remaining space by subtracting each time, so is still padding out to 23 spaces length.
|
Thanks for the bug-fix @knrc ! |
Pull Request Test Coverage Report for Build 13244372959Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Firstly, thanks very much for making this code available as it proved to be very useful for one of my projects. By way of a thank you I took a look at #350 and believe I have a solution for rebalancing the space from the merged cells. Please take a look. There may be some edge cases I have not considered, if you can think of any please let me know and I can rework the changes.
Proposed Changes
Fixes #350.