Commander.js: Indent multi-line option description in help

[mojavelinux]

If the description of an option contains line breaks, subsequent lines should be indented to match the position of the first line in the help output. Otherwise, we see something like this:

  --to-dir <dir>                                      The directory where the site should be generated.
Defaults to build/site if no destinations are specified.

[mojavelinux]

It would be great if the help was aware of the terminal width and wrapped lines that overflow intelligently, though I recognize that's a separate feature request.

[MrShoenel]

Upvoting this, the console's width can be obtained using process.stdout.columns.

[ghost]

Upvoting this as well, just stumbled into this issue myself.

[shadowspawn]

A caveat that a previous attempt at indenting multiline descriptions was pulled after it broke user formatted help: #396

[shadowspawn]

There are improvements on the develop branch for the next release (#956 and #1051).

[shadowspawn]

v4.0.0-0 prerelease published: #1067

[shadowspawn]

v4.0.0 has been released.

[mojavelinux]

The reliance on process.stdout.columns is problematic for tests (and other headless integrations), where the columns property may not be set on stdout. It would be helpful if the value could be overridden using the standard COLUMNS environment variable.

process.stdout.columns || process.env.COLUMNS || 80

May I propose a PR?

[mojavelinux]

(The output honoring the columns value is nice looking, though).

[shadowspawn]

Short version: PR welcome, thanks.

Long version

Standard, like here?

• https://pubs.opengroup.org/onlinepubs/7908799/xbd/envvar.html

For interest, in Commander's own unit tests I set process.stdout.columns directly to get consistent behaviour.

• https://github.com/tj/commander.js/blob/master/tests/helpwrap.test.js

A caveat that there is a fix coming to remove trailing space in wrapped output: #1096

[mojavelinux]

Standard, like here?

Yes. Also mentioned in the bash help.

in Commander's own unit tests I set process.stdout.columns directly to get consistent behaviour.

That only works if testing the API. It doesn't work when the command is spawned. To test my application, I'm using a CLI test framework which doesn't allow me to control how the stdio is configured. So it's out of reach. The same would be true for integrations that spawn the command using commander.js.

[shadowspawn]

Doing a little research, found one interesting complication that COLUMNS may be a shell variable in an interactive shell rather than an environment variable.

• https://stackoverflow.com/a/48016366/1082434

Potentially ok, but if find other platform/shell clashes then could use a custom name for the override like COMMANDER_COLUMNS.

[mojavelinux]

I'd honestly be fine with any environment variable name, tbh. In fact, I'd even be happy if it were a method I could override. I just need it for testing / integration, so it can be rather low level. The main issue is that the call is in the middle of several functions and I have no way to get at it so I can influence it.

[shadowspawn]

I am preferring COMMANDER_COLUMNS to be paranoid and avoid unexpected false positives with the generic COLUMNS. A bit clunky, but only being added for special circumstances. Overrides process.stdout.columns. Does that fit into your anticipated use cases?

[mojavelinux]

Yep, that would do the trick.

I still think it makes sense to extract that bit of logic as a method since it is used a couple of times in the code. It would not only make it overriddable, but also DRY. We'd likely never hear any complaints again ;)

At this point, we should probably start a new issue as this one is already closed. Would you like me to do that, or would you prefer to do it.

[shadowspawn]

Please go ahead and make a new issue. Thanks.

Extracting that bit of logic is something I considered at the time, and more logic now and more worthwhile, but I don't want to make it public. The env is the supported override mechanism.