Add construct option to customise creation of a command

[vonagam]

Pull Request

Problem

Want a way to customise creation of a subcommand through command method, mainly to use my subclass as a constructor. Without this option i would have to override command method and copy-paste code from original command method which is definitely not an optimal way.

Solution

Add construct option which if present is responsible for creating a command instance.

Can also make types more specific with generics, changing these lines:

interface Command {

  // from
  command(nameAndArgs: string, opts?: CommandOptions): Command;
  // to
  command<T extends Command = Command>(nameAndArgs: string, opts?: CommandOptions<T>): T;

}

// from
interface CommandOptions {
  construct?: (name: string) => Command;
}
// to
interface CommandOptions<T extends Command = Command> {
  construct?: (name: string) => T;
}

But wanted to keep changes small, so not included these changes to types.

[shadowspawn]

Initial reaction: probably no.

However, I'll consider it some more before posting a longer reply.

[shadowspawn]

After consideration...

1. Commander is not written with subclassing in mind. I am willing to make it more friendly, like returning this as per other PR. But I am not currently interested in taking all the changes to make subclassing a first class approach.

2. The particular approach in this PR feels more like a work-around than the ideal solution. You would need to pass construct into each subcommand but you really want it everywhere. For example, a higher level approach would be to have a factory method on Command for creating raw subcommands which you could override. (And no, I don't want to do that either!)

I suggest two possible approaches which are new in v5 (and do not require messy override of .command() itself):

a) A partial solution is to use .addCommand() with commands you create yourself. (Albeit with extra lines of code to setup each subcommand.)

b) Better, you could wrap the .addCommand() approach in a factory method in your subclass and use that instead of .command().

Fair warning, .addCommand() will give slightly different behaviours in some cases, as there are some "inherited" properties when Commander makes the comment itself. e.g. custom help flags

[vonagam]

Yeah, i understand, most likely will rewrite my own stuff with composition in mind instead of inheritance.

Here's counter-arguments though:

1. After this PR subclassing will be fully supported, no need to make any other changes, since new Command is used only in command(), every other method is ok.

2. Idea here is to support this:

class CustomCommand extends Command {
  command(name, opts) { // for simplicity args without stand-alones
    return super.command(name, { construct: (name) => new CustomCommand(name), ...opts });
  }
}

So after you redefined command you don't need to pass construct anywhere else, it will be CustomCommand all the way down.

3. I though about addCommand, but as you said is is ok if you don't modify (and don't expect anybody to modify) any transferable settings, use an executable command (_executableFile and _executableHandler are private), define a default task (_defaultCommandName is private) or customise help. And if you copy "extra lines of code to setup each subcommand" to be more compatible with usual command() then a) you will use private properties, b) in future versions internal behaviour of base command() might change and you would need to update your code again. (There is a dark dirty hack with super.command() + setPrototypeOf, but better not to think about it...)

So in summary, from my point of view, after this PR there should not be any other one connected directly to subclassing (as it will be fully supported) and it is not as work-aroundy as you think and it is only two-three lines of code (1.5 in code + 1 in types, not counting tests and possible documentation though...).

But i understand if you don't want even to think about subclassing and close this PR (feel free to do so, no hard feelings). As i said i will rewrite my stuff in composition then.

[shadowspawn]

Good comments, thanks, and thanks for being understanding too.

So after you redefined command you don't need to pass construct anywhere else, it will be CustomCommand all the way down.

Oh, that is tidier than I had in mind! I'll take another look in case that changes my thinking.

The potential .addCommand() shortcomings are interesting, because addCommand is intended to be a solid approach for separately configured commands but only just added and untried.

any transferable settings

I was thinking these would go in factory function if necessary. (i.e. using public functions to configure)

use an executable command

I was not expecting custom classes used with an executable.

define a default task

Oops, that might be an omission with .addCommand()! (And noHelp too.)

[vonagam]

I was thinking these could go in factory function if necessary. (Using public functions.)

Out of these settings:

_executableHandler // is only set in command()
_defaultCommandName // is only set in command()
_noHelp // is only set in command()
_helpFlags // has only setter helpOption()
_helpDescription // has only setter helpOption()
_helpShortFlag // has only setter helpOption()
_helpLongFlag // has only setter helpOption()
_helpCommandName // has only setter addHelpCommand()
_helpCommandnameAndArgs // has only setter addHelpCommand()
_helpCommandDescription // has only setter addHelpCommand()
_exitCallback // has only setter exitOverride()
_storeOptionsAsProperties // has only setter storeOptionsAsProperties()
_passCommandToAction // has only setter passCommandToAction()
_executableFile // is only set in command()

Only _storeOptionsAsProperties can be transferred from a parent to a child without touching private fields though a guess based on knowledge of the internal implementation:

child.storeOptionsAsProperties(parent.opts() !== parent.opts())

if addCommand() wants to be on par with command() it should have a way to communicate that those properties (except _executableHandler and _executableFile) should be transferred to an added command and if a command is default one. (Ideally command() should call addCommand().)

[shadowspawn]

I did some experimenting with a factory routine: #1191