Make copyInheritedSettings() recursive

[aweebit]

Pull Request

Problem

Pretty much all the inherited settings are such that it is only really meaningful for them to have the same values across the entire command hierarchy. However, copyInheritedSettings() only copies them at one level instead of doing it recursively. Because of that, the intended usage pattern is currently that the user adds subsubcommands only after the subcommand was added to its parent if the parent's inherited settings are meddled with (as clarified here).

Demo

const sub = new Command('sub');
const subsub = sub.command('subsub');

program
  .showHelpAfterError()
  .addCommand(sub);
sub.copyInheritedSettings(program);

console.log(sub._showHelpAfterError); // true
console.log(subsub._showHelpAfterError); // false

Solution

Make copyInheritedSettings() recursive.

ChangeLog

Changed

• Breaking: make .copyInheritedSettings() recursive (settings are now inherited by the entire command hierarchy)

[shadowspawn]

I had not thought of the situation when adding a tree.

To some extent copyInheritedSettings() is (was) low -level routine, and making it recursive means it is a bit higher level. Still thinking about it, but probably recursive is most useful behaviour and no need to make configurable in first instance.

[shadowspawn]

Somewhat cosmetic after the copy, but I suggest:

Suggested change

	command.copyInheritedSettings(this);
	command.copyInheritedSettings(sourceCommand);

[aweebit]

I wrote this here because if later some logic is added to prevent copying values for settings that had been manually set before calling copyInheritedSettings(), we want the values to be copied to the subcommands from this rather than from sourceCommand.

[aweebit]

At the risk of making the .copyInheritedSettings() behavior more confusing, I suggest making calls to it with no arguments result in inherited settings being copied from this to all its descendants.

Use case: want to change behavior of an entire command hierarchy when the root command is returned from a function with all subcommands already registered (like in https://stackoverflow.com/questions/76896860 first referenced in #1917 (comment)), for example for testing purposes (setting exitOverride like in the example etc.)

Even with changes introduced in this PR, the .commands array would still have to be iterated to achieve the desired result. If the new suggestion is accepted, one call will be enough.

Could also dump this PR if the suggestion is accepted. The change would be non-breaking then.

[shadowspawn]

Currently copyInheritedSettings does one simple thing, which I like. It exposes what used to be an internal step so authors can use it for assembling commands. I don't like making routines more flexible (and complicated) without a clear vision. In your case, you are already wondering about two different implementations. My instinct is not clear enough yet (i.e. I want to see another/other issues).

Given it is easy enough for author to implement exactly what they want, and I think a different approach would be better for the .exitOverride() example, I am going to close this for now.

The apply-settings-after-creating-program situation might be handled by something like:

program.recursiveSet((cmd) -> {
   cmd.exitOverride();
   cmd.helpOption('-a, --assist', 'assistance');
});

[aweebit]

The apply-settings-after-creating-program situation might be handled by something like:

program.recursiveSet((cmd) -> {
   cmd.exitOverride();
   cmd.helpOption('-a, --assist', 'assistance');
});

.recursiveSet() would just have to do exactly what a .copyInheritedSettings() call with no arguments would do in my suggestion, just after calling the callback function. There is no need to make it so complicated.

In your case, you are already wondering about two different implementations.

It's a good thing this PR was kept open long enough for me to come up with a better idea :)) I think just adding support for the call with no arguments is great because the suggested behavior is likely what most users really want when they use .copyInheritedSettings(), but at the same time, the change wouldn't be breaking and finer control would still be possible in cases where it's needed.