Skip to content

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Jul 4, 2020

Pull Request

Problem

  1. can not add a splash screen/title/banner to the help
  2. can not add a global epilog
  3. event listener is a little clunky and unfamiliar to use
  4. help is displayed on stdout even when being displayed as an "error"

See #1225 for collected custom help issues, and #997 for error output.

Solution

Add .addHelpText() which takes a position as first parameter, and a string to add or a function returning a string. Position is 'before' or 'after' to affect just this command, and 'beforeAll' or 'afterAll' to affect this command and all its subcommands.

For example:

program.addHelpText('afterAll', '/nSee the web site for more information');

Also:

  • .help() and .outputHelp() can be passed { error: true } to write to stderr.
  • Deprecate passing callback to .help() and .outputHelp()
  • Deprecate .on('--help')

ChangeLog

  • use .addHelpText() to add text before or after the built-in help, for just current command or also for all subcommands (Expand help customisation with .addHelpText #1296)
  • deprecated callback parameter to .help() and .outputHelp() (removed from README)
  • deprecate .on('--help') (removed from README)

@shadowspawn shadowspawn marked this pull request as draft July 4, 2020 06:56
@westhom
Copy link

westhom commented Jul 28, 2020

Hi there, one thing I badly wanted from commander was the ability to group commands and options in help output, for readability. I implemented it as a quick hack here: https://github.com/westhom/commander.js/compare/c5a5e7b7..86a1980b.

For commands, you can call program.command(...).helpGroup(N), where N is a group index the command belongs to. For options it wasn't as clean, set through a new param: program.option('-k', 'set K', undefined, undefined, N). The help generator methods sort commands/options by their group before formatting the line. By default everything belongs to group 0 and if no other groups exist the help output is normal. Example:

Usage: help-groups [options] [command]

Options:
  -a                 set A
  -b                 set B
  -c                 set C
  
  -q                 set Q
  -r                 set R
  -s                 set S
  
  -h, --help         display help for command

Commands:
  login              log in
  logout             log out
  
  read <process>     read it
  update <process>   update it
  delete <process>   delete it
  refresh <process>  refresh it
  
  publish            publisher
  unpublish          unpublisher
  
  help [command]     display help for command

I was going to open a PR for this but it looks like help is being revamped. It could be nice to have a grouping option like that.

@shadowspawn

This comment has been minimized.

@westhom

This comment has been minimized.

@shadowspawn shadowspawn added the semver: major Releasing requires a major version bump, not backwards compatible label Aug 2, 2020
@shadowspawn
Copy link
Collaborator Author

To be safe, the change to use stderr for displaying the help in response to an error makes it a semver major change.

The other changes are backwards compatible, as the --help event and the .help(cb) callback are deprecated but not removed.

@shadowspawn shadowspawn changed the title WIP: Expand custom help features Expand custom help features with more events Aug 8, 2020
@shadowspawn

This comment has been minimized.

@shadowspawn shadowspawn changed the title WIP: Expand help customisation with .addHelpText Expand help customisation with .addHelpText Sep 4, 2020
@shadowspawn shadowspawn marked this pull request as ready for review September 4, 2020 22:06
@shadowspawn
Copy link
Collaborator Author

I am happier with how this is looking now with an explicit routine which is more readable than an event listener, and leaving the ability to override the built-in help out of this routine.

@shadowspawn shadowspawn removed their assignment Sep 6, 2020
@shadowspawn
Copy link
Collaborator Author

I deprecated the callbacks to .help() and .outputHelp() as this PR originally included being able to override the built-in help too. That is no longer included, but I would still like to call the callbacks deprecated so can remove them in future!

@abetomo
Copy link
Collaborator

abetomo commented Sep 9, 2020

Could you merge develop into release/7.x?
This includes the 6.1.0 differences.

@shadowspawn
Copy link
Collaborator Author

Oops, will do. I was merging 6.1 into the PR so the diff against develop looked better, missed doing 7.x too.

@shadowspawn
Copy link
Collaborator Author

I have merged develop into release/7.x.

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Sep 9, 2020
@shadowspawn shadowspawn added this to the v7.0.0 milestone Sep 9, 2020
@shadowspawn shadowspawn merged commit d26a26d into tj:release/7.x Sep 9, 2020
@shadowspawn shadowspawn deleted the feature/on-help branch September 13, 2020 07:30
@shadowspawn shadowspawn mentioned this pull request Oct 14, 2020
@crystalfp
Copy link

Only a (minor) issue in the new version:

  1. .addHelpText("after", "\n") adds two blank lines after help.
  2. .addHelpText("after", "") adds nothing after help.
  3. .addHelpText("after", " ") adds one blank line after help.

A little confusing. If case 1 should behave as console.log() then case 2 should add one blank line after help. Instead, if the function behaves like write() then case 1 should add one blank line, in my opinion.

Besides this minor issue, the new version is excellent. Thanks!
mario

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 15, 2021

Case 2 is a special case. If there is an empty string then nothing is output. This is to support a function generating the string deciding that there is nothing to be displayed.

@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Jan 16, 2021
@crystalfp
Copy link

OK, understand. Maybe adding a line to documentation stating this could be useful. Something like this at the end of the Custom help paragraph: “If the string or the output of the function is an empty string, nothing is output, otherwise the string is output followed by a newline”.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: major Releasing requires a major version bump, not backwards compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants