Rework Help.wrap()

[aweebit]

Pull Request

Problem

I first thought of reworking wrap() when I wanted to wrap lines of text in a custom help message in such a way that only the first output line is indented. (At that moment, I did not think of manually indenting the lines before passing them to the function for some reason.)

I also did not like the fact text was not wrapped whenever the decision was made to not indent it due to insufficient width – I see no reason why the all-or-nothing principle had to be applied here.

Not being able to apply wrapping just because some lines are indented manually by even just a small amount also seemed pretty lame.

Solution

An attempt to make just one change to solve my problem ended up in a (more or less) complete rework of wrap(). The new function can be used to format entire tables, while remaining for the most part compatible with the current implementation.

(Updated) new method signature

with my made up "skippable" argument syntax to fit everything in one signature.

interface WrapOptions {
  leadingStrLength?: number;
  minWidthGuideline?: number;
  preformatted?: boolean;
  overflowShift?: number;
  globalIndent?: number;
  columnGap?: number;
}

wrap(
  str: string,
  width: number,
  [leadingStrLength: number = 0,
  [minWidthGuideline: number = 40,
  [preformatted: boolean]]],
  options?: WrapOptions
): string;

Example

const { Help } = require('.');

const table = `
Country      Languages
-------      ---------
Argentina    Spanish
Canada       English, French
Germany      German
Switzerland  German, French, Italian, Romansh`;

const newColumn = `
Capital
-------
Buenos Aires
Ottawa
Berlin
(Not mentioned in the constitution, but Bern is the de facto capital)

It is cool to learn about the world!

  I am not pre-formatted!`;

const result = `
  Country      Languages                         Capital
  -------      ---------                         -------
  Argentina    Spanish                           Buenos Aires
  Canada       English, French                   Ottawa
  Germany      German                            Berlin
  Switzerland  German, French, Italian, Romansh  (Not mentioned in the
                                  constitution, but Bern is the de facto
                                  capital)

                                  It is cool to learn about the world!

                                    I am not pre-formatted!`;
/*
..,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,:::::::::::::;;/////////////////////////////

      . - `globalIndent`
, and : - `leadingStrWidth` inferred from `str.slice(0, leadingStrLength)`
      ; - `columnGap`
      / - wrapped column width = `(width - 1)` minus everything before
: and ; - `overflowShift` (negative)
*/

new Help().wrap(
  table + newColumn, // input string
  80, // output width plus 1
  table.length, // length of part representing the table to expand
  20, // minimum width to allow indentation (allowed here)
  {
    preformatted: false, // explicitly specifying lack of pre-formatting
    overflowShift: -15, // amount to shift overflow by
    globalIndent: 2, // global indentation amount
    columnGap: 2 // gap to insert between table and new column
  }
) === result; // true

Despite table support and many other changes not being essential for internal use in Commander, they make wrap() a quite powerful library function that can be used to format custom help messages and more, while at the same time preserving the original functionality and not introducing any performance penalties (although I haven't measured it, getting rid of lazy quantifiers in regular expressions might have even improved it).

Footnotes used in change descriptions

• ¹ Cosmetic change
• ² Minor functionality change
• ³ (Relating to a) breaking change
• ⁴ (Relating to a) new feature – all backwards compatible

Changes include:

• ² width can now be infinite and is considered such if undefined is passed (could easily happen in formatHelp() if it did not resort to 80)
• ¹ indent has been renamed to leadingStrLength to better reflect the argument's true meaning and avoid confusion with other values affecting indentation
• ² leadingStrLength now defaults to 0
• ⁴ support for multiline column merging like in the example above that boils down to treating first leadingStr.split('\n').length lines the same way only the very first line is treated in the current implementation (where leadingStr = str.slice(0, leadingStrLength)). Indentation amount is equal to the length of the longest line in leadingStr (leadingStrWidth), which degenerates to leadingStrLength (or currently indent) if there is only one line
• ⁴ overflowShift can be specified to shift overflowing column text by some amount. Negative values are allowed, and therefore it can be used to solve my original problem of indenting only the first line, see details below
• ¹ minColumnWidth has been renamed to minWidthGuideline, again, to better reflect its meaning:
  • Column omitted because only overflowing text width is compared to this value, and it is not entirely clear whether the overflow can be considered part of the column when it is shifted
  • Guideline added because even when no indentation is used, the width could still be insufficient, and we would not be able to do anything about it
  • Overflow not added instead of Column because the fact we use it just for overflow does not mean the value of this guideline is bound to this context. (For example, it could be used to wrap entire text columns to next "column line" when the remaining width for next column rendering is insufficient – check CSS flex-wrap property to see what I mean)
• ⁴ globalIndent can be specified to indent all non-empty lines by some amount, except when there is not enough space to display overflowing lines adequately, in which case no indentation to such lines is applied. As a result, formatHelp() does not have to postprocess the lines by prepending 2 spaces anymore, and the actual terminal width can be passed to wrap() without modfications. But the fact only non-empty lines are indented has the following breaking change as a consequence:
• ³⁴ breaking: since formatHelp() now relies on wrap() for applying indentation, empty lines in pre-formatted argument/option/command descriptions are not indented by 2 spaces anymore (test updated)
• ⁴ columnGap can be specified to adjust distance between text column defined by leadingStr and one defined by the remaining part of str (in the traditional use case, it would be the distance between term and description). Again, formatHelp() can now simply pass the item separator to wrap() instead of having to deal with it by itself
• ⁴ preformatted can be specified to request treatment of text as (not) manually formatted (opens possibilities to extend API that could solve Wrapping the second and following line in a multiline description doesn't work if the line starts from space #1822)
• ^2/3/4 error messages are thrown if arguments that are expected to be non-negative are passed negative values. Especially important for leadingStrLength, since it is exclusively used in calls to strings' slice() method which does not fail on negatives
• ⁴ if the full indent globalIndent + leadingStrWidth + columnGap + overflowShift is negative, the non-overflowing output part will be indented by the absolute value of this number in addition to globalIndent. This is required to make room for a negative overflowShift, so this is the solution to the original line indentation problem. For example, let's say leadingStrLength = 0, overflowShift = -10, globalIndent = 3 and columnGap = 0, then the first line will be indenten by -(3 - 10) = 7 additional spaces so that it would appear that all other lines are shifted by -10
• ³ breaking: if width is insufficient to display overflow adequately, overflowing lines are not indented but still wrapped (test updated)
• ⁴ there are now two regular expression used for line matching, one for non-overflowing and one for overflowing text
• ³ the regular expression structure has been reworked: newline part and lazy quantifier have been removed, part is captured for later manual processing, other groups are non-capturing, g flag dropped, y added. Some breaking changes, too:
• ³ breaking: the regular expressions now never match a line of more than width - 1 non-breaking characters. As a result, overflowing words are now wrapped (tests added)
• ³ breaking: all whitespaces at the end of a matched line are collapsed, so wrap('a b', 2) === 'a\nb' (test added)
• ⁴ if word does not fit in column in non-overflowing part and overflowShift is negative, the algorithm will try to place it in overflow instead of splitting

Additional infos

• Only 2 tests had to be changed 😊
• Despite all the new complexity, formatHelp() code did not actually have to be altered and would work the same as before (and the first mentioned breaking change would not be there), but I decided to let wrap() do the job of inserting the "item indent" and "item separator" because
  • it now can, so why not make use of that
  • width argument value is the entire available width which is nice
  • I wanted to illustrate the new use pattern
  • there is no need to indent empty lines anyway

To-dos

• Tests for new features
• Error message(s)
• Extend API to allow explicitly specifying argument/option/command descriptions and custom help as (non-)pre-formatted?
  • Added preformatted property to Help class instead

Questions

• Can the appearance of extra lines with no text as described here be reproduced now with the new code if width-1 is replaced with width in the regular expressions? If not, I think it would make sense to do the change so that the function behavior is more intuitive.

Footnotes

1. Cosmetic change ↩ ↩² ↩³

2. Minor functionality change ↩ ↩² ↩³ ↩⁴

3. (Relating to a) breaking change ↩ ↩² ↩³ ↩⁴ ↩⁵ ↩⁶ ↩⁷

4. (Relating to a) new feature – all backwards compatible ↩ ↩² ↩³ ↩⁴ ↩⁵ ↩⁶ ↩⁷ ↩⁸ ↩⁹ ↩¹⁰ ↩¹¹

[shadowspawn]

There is a pretty epic amount of information here. Thanks for the detail on the changes.

A word of warning. If your end goal is an accepted Pull Request, I suggest you ask some questions before launching into major changes and rewrites, to avoid disappointment.

However, if your end-goal is sharing some interesting work you did that might be accepted or used in some way, that is fine. I look forward to looking through it! I suspect it might be beyond what I want to pull into Commander, but is still likely to have some interesting ideas that we might want to use in smaller changes.

The existing wrap API is a bit magical. It evolved out of the code usage and I have had one go at making it easier to understand, with limited success.

For interesting related reading, @cravler has made lots of suggestions and good feedback for Commander, driven by making it easier to do custom (and fancy) help: https://github.com/tj/commander.js/pulls?q=is%3Apr+involves%3Acravler

[shadowspawn]

I think a nicer of handling multiple optional parameters is to pass them in an options "bag". This both allows specifying any one of the optional values (instead of all the values up to the one desired), and provides some self-documentation in the call.

e.g.

help.wrap(fullTest, 23, { minWidthGuideline: 20 })

[aweebit]

Yeah, all the indents simply listed one after the other can be quite confusing, so I agree with you, except maybe preformatted should be additionally available as a separate argument, just like minWidthGuideline is now. Both are special because they are in essence universal settings that are supposed to have the same value in each call in any reasonable application, so ideally, they should only be used directly when overwriting wrap() in a subclass. Because of that similarity and since minWidthGuideline is already available as a separate argument, I think preformatted should be, too. We could also make them both properties of the Help class instead, so that the values could be configured with Command.configureHelp().

Anyway, I got stuck trying to write JSDocs for the overloads with an options parameter. Apparently, VS Code does not understand overloaded functions well, so it led me to create this issue: https://github.com/microsoft/vscode/issues/187663.

[aweebit]

fbddb6b, 4a08544

[aweebit]

My goal is both, actually. I would not be spending so much time on it if it was not fulfilling on its own, but of course it would be nice to see the PR or at least some of the changes accepted into the project :)

This time, I really did not know where it was going. I started with just a couple small experiments for myself that were not supposed to become anything big, and then just kept adding new stuff because something always felt missing, and I kept doing that until it got to this point where the method looks like it's got all it needs. I guess I just got inspired and decided to keep going instead of asking questions.

This PR is totally not helping to make wrap() easier to understand haha. But I actually quite like the table example I put together, and especially the marking in the comment. I hope it helps at least somewhat to not get lost in all those new arguments.

I see now that I put too much emphasis on the word "table" when naming variables and writing JSDoc and this PR. It makes the suggested changes seem to be a bigger deal than they actually are, so I have now tried to get rid of as many table references as possible. The fact is that the table functionality was just a very natural extension of code I ended up with, so I could not reject the idea of adding those 11 extra lines. It is definitely one of the easiest to implement and one of the least important new features. so I will not be disappointed at all if it is not accepted.

Can't say that about the changes that do not introduce any new features, though. All changes I have now marked with [2] and [3] in the PR description seem very reasonable to me and I really cannot think of a reason why they should be rejected, except maybe the suggested regex structure is debatable.

The new feature marked with [4] can be summarized in the following four classes:

• preformatted (seems like something that's been missing for a long time although elementarily implementable)
• globalIndent and columnGap (both relieve formatHelp())
• overflowShift (ideally requires globalIndent so that first line does not have to be indented manually if no leadingStr is provided)
• column merging

[aweebit]

Oops, I accidentally posted too early again, but I was almost finished anyway. I put this feature class list together to reiterate that the changes I am suggesting are not actually such a big deal, even if the full list looks a bit scary. As for their importance, I think there really needs to be a way to control the pre-formatting assumption, at least by means of subclassing, so I would rank preformatted as the most important missing feature. All the rest are just enhancements that would be nice to have, with column merging being the least important one in my opinion. The code just did not feel complete without it.

[shadowspawn]

The basic idea of preserving the pre-formatted text was to preserve text that author carefully formatted themselves, in particular before wrapping was introduced. Terminal width 80.

% node preformatted.js --help
Usage: preformatted [options]

Options:
  --pre-formatted <value>  long description follows which
      Commander guesses is pre-formatted, perhaps to make use of full
      terminal width to fit on the target display nicely
  -w, --wrap               long description follows:
                           1) Commander wraps but preserves the line breaks,
                           which may create some short lines
                           2) but does at least start the lines where desired
  -h, --help               display help for command

and with pre-formatted parameter in PR set to false get:

% node preformatted.js --help
Usage: preformatted [options]

Options:
  --pre-formatted <value>  long description follows which
                               Commander guesses is pre-formatted, perhaps
                           to make use of full
                               terminal width to fit on the target display
                           nicely
  -w, --wrap               long description follows:
                           1) Commander wraps but preserves the line breaks,
                           which may create some short lines
                           2) but does at least start the lines where desired
  -h, --help               display help for command

Example code used:

const program = require("commander");

program
  .option("--pre-formatted <value>", `long description follows which
    Commander guesses is pre-formatted, perhaps to make use of full
    terminal width to fit on the target display nicely`)
  .option("-w, --wrap", `long description follows:
1) Commander wraps but preserves the line breaks, which may create some short lines
2) but does at least start the lines where desired`)

program.parse(process.argv);

[shadowspawn]

I think the most likely response to displaying the help in a small terminal window is going to be drag the window bigger. This is partly why the formatting is abandoned if too narrow. If the text is fully wrapped in a narrow window, it will need to be displayed again when window is made more sensible size.

I had a look at what man does, and it has an interesting related but different fallback. It wraps the text to the smallest supported width. That way when when you drag the window bigger, the text shows up as nicely wrapped when get to or past that minimum width. I think that is better than unformatted for the window-resize usage case.

[aweebit]

I think the most likely response to displaying the help in a small terminal window is going to be drag the window bigger. This is partly why the formatting is abandoned if too narrow. If the text is fully wrapped in a narrow window, it will need to be displayed again when window is made more sensible size.

I don't think it is common to type commands in a narrow terminal window and find out it is too narrow only after the output has been displayed, so the real use for minWidthGuideline is when the first column of text gets too big, due to an --extraordinarily-long-option-name <and-an-unexpectedly-big-option-argument-name> for example. But in this case, simply allowing the overflowing text to take the entire width by giving up indentation is enough to solve the problem. And if I have to choose between nice wrapped output or output being resizable because no line breaks were used, I would definitely choose the former.

I had a look at what man does, and it has an interesting related but different fallback. It wraps the text to the smallest supported width. That way when when you drag the window bigger, the text shows up as nicely wrapped when get to or past that minimum width. I think that is better than unformatted for the window-resize usage case.

Do you think this is what we should do if even the full width does not reach minWidthGuideline? One line would be enough to implement:

width = Math.max(width, minWidthGuideline);

But then the default value for minWidthGuideline should be much smaller, 15 for example. Or we could introduce yet another new variable for this matter (minWidth? And then maybe rename minWidthGuideline to minWidthToIndent for clarity)

[shadowspawn]

I stumbled across an error when the window is narrower than the first column:

        const { 0: match, 1: line, index } = regex.exec(columnText);
                   ^

TypeError: Cannot destructure property '0' of 'regex.exec(...)' as it is null.
    at Help.wrap (/Users/john/Documents/Sandpits/commander/my-fork/lib/help.js:614:20)

[shadowspawn]

Overall I feel this is a lot of new code with a lot more knobs and dials available to subclassing. There are a variety of ideas and controls for formatting. But there have not been only a few issues opened about wrapping, and as a fallback people who want custom behaviour can subclass and do what they want themselves.

I'm happy to leave the PR open for a month and think about it some more, and see if any feedback or related issues come up, but currently leaning towards no. (It has only been open for a couple of days, but I didn't want you to put in further work under the expectation it would be accepted.)

I appreciate that you thought about and commented on what changes would be breaking.

[shadowspawn]

A musing from some research tonight. man pages typically have a block per option. A little unusually, Git even does this for its comprehensive command-line help. I think this style reduces terminal width problems, and is good for long descriptions.

$ git help commit
...
       -a, --all
           Tell the command to automatically stage files that have been modified
           and deleted, but new files you have not told Git about are not affected.

       -p, --patch
           Use the interactive patch selection interface to choose which changes to
           commit. See git-add(1) for details.

[aweebit]

I stumbled across an error when the window is narrower than the first column:

It turned out to be really easily solvable by a recursive wrap() call, partially due to the infrastructure built for column merging. The result is also much better than it used to be (in my opinion at least):

const term = '--extraordinarily-long-option-name <and-an-unexpectedly-big-option-argument-name>  ';
const description = 'DESCRIPTION that is also quite big so it overflows';
console.log(new Help().wrap(
  term + description,
  process.stdout.columns,
  term.length,
  0,
  { globalIndent: 4, columnGap: 4 }
));

With console output being

    --extraordinaril
    y-long-option-na
    me
    <and-an-unexpect
    edly-big-option-
    argument-name>
DESCRIPTION that is also
quite big so it
overflows

with the new code, and

--extraordinarily-long-o
ption-name <and-an-unexp
ectedly-big-option-argum
ent-name>  DESCRIPTION t
hat is also quite big so
 it overflows

with the old when the width is 25.

[aweebit]

Overall I feel this is a lot of new code with a lot more knobs and dials available to subclassing. There are a variety of ideas and controls for formatting. But there have not been only a few issues opened about wrapping, and as a fallback people who want custom behaviour can subclass and do what they want themselves.

I just want to point out one more time that the changes marked with [2] and [3] are just reasonable defaults, with ones marked with [3] currently requiring rewriting the entire method when subclassing, which no one would want to do. Same applies to rewriting the entire method just to get rid of the manual formatting assumptions.

I'm happy to leave the PR open for a month and think about it some more, and see if any feedback or related issues come up, but currently leaning towards no. (It has only been open for a couple of days, but I didn't want you to put in further work under the expectation it would be accepted.)

Okay, perfect, I will not hurry with adding tests then. And I would be happy to hear feedback from other people during the next month. We cannot be the only ones reading all of this :)

I appreciate that you thought about and commented on what changes would be breaking.

And I appreciate you investing your time in reviewing the changes, even if the PR does not end up accepted.

[shadowspawn]

Closing after a month as per: #1904 (comment)

This sort of flexibility might go into a library on its own, which for interest is an approach Yargs has taken: https://github.com/yargs/cliui