Support column width control (MaxWidth configuration)

[tooda02]

This supports specifying the maximum width of columns and of the entire output line. Columns with data exceeding the maximum width are broken at word boundaries into two or more lines. See the README for details.

[ryanuber]

This is some pretty tricky stuff. It will take me a bit to fully understand what is going on in the code. This seems to add a fair amount of complexity, so I'm not sure yet if I'll end up merging it. I'll think about this more. Thanks!

[tooda02]

Hi Ryan,

No problem - thanks for reviewing the update. If you have any questions,
please feel free to email me directly - columnize at tootill dot net.

BTW, try it with data exceeding the width of the terminal - that's what led me to develop the change.

Regards
David Tootill

On Sat, Oct 10, 2015 at 2:45 PM, Ryan Uber notifications@github.com wrote:

This is some pretty tricky stuff. It will take me a bit to fully
understand what is going on in the code. This seems to add a fair amount of
complexity, so I'm not sure yet if I'll end up merging it. I'll think about
this more. Thanks!

—
Reply to this email directly or view it on GitHub
#6 (comment).

[tooda02]

@ryanuber Can you please take another look at this PR? I've updated it to have a separate OutputWidth configuration and to support OutputWidth: AUTO to set width to the actual width of the console. IMO, these changes make both the code and the documentation easier to follow.

Anyway, please let me know whether or not you plan to merge this.

[ryanuber]

I took another look through this. I think that specifying column width could be useful in some cases, but I don't think that fork/exec should be used in this library for anything. I also feel like we are inlining a lot of extra code into getWidthsFromLines, and I'm not sure why the default config is made to be mutable in this PR. Can you explain that? I'm still interested in the column width-restricted fields, but I think we need cleaner separation, and ideally keep all of the functions pure for simplicity.

[tooda02]

Thanks for commenting. Just for background, this PR stems from use of Columnize in a large scale CLI - thousands of lines of code - that's part of a new commercial product. The requirement for being aware of terminal width cropped up early - Windows terminals in particular are usually only 80 characters wide (it's awkward and non-obvious how to widen a Windows terminal - just dragging the edge doesn't work), and columnized output looks terrible when broken in an arbitrary place.

Anyway, for your specific questions:

1. fork/exec shouldn't be used in this library for anything There's no use of fork. Exec is used only when the user explicitly requests it by specifying OutputWidth=AUTO. I don't think there's any way of determining terminal width without issuing an OS command, but I'll be happy to work with a different method if you know one. IMO, automatically sizing to terminal width is a key feature - perhaps the single most important in the PR - and I'd be reluctant to disable it. What is your reason for disliking exec?
2. inlining a lot of extra code into getWidthsFromLines I don't see any extra code in this function (please point out anything you see as redundant), so I'm assuming that you'd asking for it to be broken into smaller pieces. I've committed a change doing this.
3. why the default config is made to be mutable In a large scale application, there can be many uses of Columnize. It's useful to be able to be able specify a set of defaults in one place and override them as required. This also makes default settings easier to change if requirements change, and ensures output is consistently formatted across the CLI.

It seems to me that cleaner separation, and ideally keep all of the functions pure is a restatement of item 2 above. Please clarify if this is an incorrect assumption.

[tooda02]

@ryanuber It's been a month, so it seems like you don't want to respond. I'm not sure what the issue is - width control is aligned with the purpose of the package and will be useful to users that need it, but has zero impact on users that don't, including all existing users, It seems to me it's well-documented and easy to use (let me know if you disagree), and I've tried to update the code to conform to your preferences.

If you intend never to merge this, I can withdraw the PR and maintain my version independently as a hard fork. Please comment.

[ryanuber]

Hi @tooda02,

Apologies for delays from my end, my daily work revolves around GitHub with some extremely active repositories, so I occasionally miss things from my personal projects context. Sorry about that.

So I think that we should keep this library focused on formatting the output and not have it reach into OS-specific command line programs. I'd rather avoid checking the runtime and regex-matching command output, not because I think the approach is incorrect, but because that feels like a point for logical separation, where we could have an additional package to do this for us with an interface like TerminalDimensions() (height, width int). I could see that being useful elsewhere as well.

I'd argue against making the package-global default configuration mutable, whether by a direct set, or via a setter. What's weird in this case is that any library imported into any given application can mutate the default configuration, which other callers may not be expecting. I would prefer to keep the default configuration immutable, as it was before, and have callers continue passing in their own configuration if they want something other than the defaults. That is why we have the SimpleFormat() and Format() methods with different signatures.

I think we also have some edge cases in here. Because the lines are only truncated and appended to the next on the last column, output with many wide columns, or a short last column, won't be formatted properly.

I think for the short term it might be worth it for you to do a hard fork since I don't think we are ready for a merge at this point, and I'd hate to be blocking you on this. Hope that helps. I do sincerely appreciate the efforts here, thank you!

[tooda02]

Hi @ryanuber,

Thanks for the thoughtful comments. I've committed another change that hopefully addresses them:

• Your comment about terminal width being a point of logical separation is well-taken. As it turns out, there now is such a package, Demille/termsize. This was created April 2015 (after I started working on columnize changes, which is why I didn't use it in the first place). It's based on code extracted from termbox-go, which I did look at, but rejected as too heavyweight for this application. Demille's package includes just the termsize code, which is implemented with syscall rather than os/exec. I've vendored it into my commit, so columnize is protected from changes to its dependency.
• You raise a valid concern about SetDefaultConfig() and I've removed it with this update.
• As for lines are only truncated and appended to the next on the last column, the feature is actually more flexible than you're giving it credit for. The caller can restrict the width of any or all columns with the MaxWidth configuration element (for an example, look at the "expected" value in the TestMaxWidth unit test). It's true that changes triggered by OutputWidth apply to the rightmost unrestricted column (it defaults to the last, not the next to last, and can apply any column, not just the last one). While there is certainly room for a future enhancement that would allow the OutputWidth restriction to be distributed among several columns, single-column support covers the important use case where one column is a description. One example of this is help output. In fact, it was help formatting that first got me started on controlling output width.

Anyway, there's no rush on the merge and I have no problem with keeping the repo a soft fork until you make your decision.

[tooda02]

@ryanuber Just wondering if you'll have a chance to look at this anytime soon.

[tooda02]

@ryanuber It's been a couple of months since I responded to your comments on this PR - when do you think you'll have a chance to review the response?

[tooda02]

Closed in favor of #8, which is identical except for the repo owner.