[WIP] Forward subcommands (action-like subcommands)

[a-fas]

Pull Request

Problem

Addressing #764, briefly:

• Adding subcommands that are not spawned and don't require separate files
• Adding collectAllOption feature to cleanly collect command options from all levels of subcommands (mainly, however can be used a a clean copy listing all option values even those which were not specified)

Solution

• Command.forwardSubcommands (code published in Subcommands syntax and usage #764)
• Command.collectAllOptions
• Command.useSubcommand - semantically cleaner version of forwardSubcommands

ChangeLog

Action-like subcommands: specify subcommands with callback actions without separate process spawning, see use-subcommands.js in examples and read Sub commands (handlers) section of the readme

[shadowspawn]

Thanks @a-fas

It will take me a while to have a look through this. I am interested in whether this can be made a default behaviour, rather than a special mode. Do you think that could be feasible with the approach you took, or did you discover certain behaviours mean you need to explicitly switch modes? (Asking a light question in case you have insights.)

[a-fas]

Hmmm, Not sure I clearly understood the question :) And what do you mean with "default behavior".

However! Working on this feature I considered it more as a side-feature. But looking from a higher perspective I think I have one better idea. I'll post the full answer in the original issue (#764 ) as it maybe worth a wider discussion.

[jdalrymple]

Noticed a problem when using --help with a subcommand. Could just be local so im investigating, but basically calling command subcommand --help returns 'Maximum call stack size exceeded' error :/

[a-fas]

Sorry for long silence, had been rather busy. I reworked the code, this version seems much cleaner semantically to me. Have a look. @shadowspawn

• rebased onto master
• Docs, tests, and examples are updated as well. And the _exit feature adopted.
• plus a bit more sophisticated auto help output: also triggers if command has actions but no args passed (maybe this logic should be unified globally)

[shadowspawn]

I am looking forward to digging through this. Thanks @a-fas .

I see several small things I had been wondering about too, good prompt to follow through with some small improvements. Like:

// const { Command } = require('commander'); << would be in a real program
 const { Command } = require('..');

[a-fas]

Not sure I understood the last point. Do you mean removing the commented // const { Command } ...? I actually left it intentionally. This is an example and require('..') might be confusing from a quick glance. No big deal of course, but that was kind of care about the "users" :) However if you insist, I can remove, no prob, just let me know.

[shadowspawn]

Do you mean removing the commented

No, I quoted that because I liked it. I have looked at the the require('..') in the various example files and wondered about changing them or adding comments, like you did. The '..' is handy for running the files directly from a checkout of the repo, but does not look like a "real" program or allow copy-and-paste without editing. So I was interested to see you were thinking something similar.

[a-fas]

Yeah, copy-paste is exactly what I had in mind with the example :)

One further consideration regarding collectAllOptions ... maybe it would be better to unify it into opts method: making an optional param deep = true/false or collectFromParents = true/false or similar. Might be quite organic. Especially having the new options location logic in the dev branch. What do you think ?

[shadowspawn]

One further consideration regarding collectAllOptions ... maybe it would be better to unify it into opts method: making an optional param deep = true/false or collectFromParents = true/false or similar. Might be quite organic. Especially having the new options location logic in the dev branch. What do you think ?

Short version: Yes.

Long version

I have been wondering that too since you first proposed an option collection routine. In addition I am wondering about a call to make it the default behaviour so it can be used when the action handler is only passed the opts and not the Command object (the new opt-in behaviour on develop branch). I expect people want it one way or the other most of the time.

There is an ambiguity about what to do when option defined at multiple layers. Command line parsing works from parent down to children, but child could have a default value which still gets defined. I think parent value overriding child value matches the parsing, and the problem only comes up in a use-case which is not explicitly supported.

This can go in a separate Pull Request.

[shadowspawn]

plus a bit more sophisticated auto help output: also triggers if command has actions but no args passed (maybe this logic should be unified globally)

That is something I have been wondering about separately to unify the action-handler and executable-handler behaviours, and work more often the way people expect. I haven't worked through the details yet though, and recommend don't attempt the unified behaviour in this PR. One thing at a time...

Related: #1088

Thanks for comment.

[shadowspawn]

Today I have been doing lots of reading of this and existing code and #245. I am going to try some ideas for a different approach which uses pieces of all of them. I'll see how it goes and report back...

[a-fas]

Quick thoughts from my side

opts

• I think passing the opts instead of the commander instance to an action handler is a very good idea. Current behaviour gives unnecessary powers to the action handler
• in the collectAllOptions the overriding prefers children. My logic here was that child opt was more specific and so more important. But, yes, the default values ... I didn't think about it. Is there a flag that indicates that an opt was populated by user or taken from the default ?
• just to wonder - maybe pass a wrapper object, which exposes both opts per level and a collection routine for simpler access? Maybe also have a param in opts to get same wrapper ?

separate Pull Request: sure 👍. This however makes collectAllOptions kind of temporary solution. Is it ok ?

unified behaviour of the output: sure, I also think it is a separate concern.

[shadowspawn]

Thanks for quick thoughts @a-fas

collectAllOptions and override order: if Commander support positional options in the future, then child overriding parent and taking the most specific option makes sense, as per your logic. There isn't a way to tell where option value came from.

[shadowspawn]

I have added #1155 for the collectAllOptions/opts idea.

#1149 includes direct nested subcommands, .addCommand, and consistent error handling (inspired by this PR).

Closing this PR.

Thank you for your contributions.