fix: resolve options against the active command

[AmirSa12]

closes #153

• Parse arguments left-to-right while tracking the active command.
• Resolve option arity within the correct root or subcommand scope.
• Add regression tests for alias and long-option collisions.

[changeset-bot]

🦋 Changeset detected

Latest commit: 81e709a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@bomb.sh/tab	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@bomb.sh/tab@154

commit: 81e709a

[shadowspawn]

Did you see my comment? I think the existing implementation matches Commander default behaviour, root options mask subcommand options. #153 (comment)

However, I am not too worried if you do go ahead and change behaviour:

1. Having same option at multiple levels in the command hierarchy is not common and is inherently a bit ambiguous. High level, user would be better to avoid it! 😬

2. Commander integration could omit options from subcommand completion if they are masked, to match the implementation. (But potentially not worth effort, see (1)!)

[AmirSa12]

Did you see my comment? I think the existing implementation matches Commander default behaviour, root options mask subcommand options. #153 (comment)

However, I am not too worried if you do go ahead and change behaviour:

1. Having same option at multiple levels in the command hierarchy is not common and is inherently a bit ambiguous. High level, user would be better to avoid it! 😬
2. Commander integration could omit options from subcommand completion if they are masked, to match the implementation. (But potentially not worth effort, see (1)!)

Hey John!
Thanks for your review and sorry I missed your earlier comment 👀 I was not aware of Commander's enablePositionalOptions() behavior and you are right that option resolution depends on the parser and its configuration.
but the existing tab behavior also only partially matches Commander's default:

// root:  -c, --config <file>  (takes a value)
// serve: -c, --color          (boolean)
// serve: [env]                (positional)

commander:

$ app serve -c root.json

tab:

$ app complete -- serve -c ''
:4

tab first uses the root --config option to decide that -c needs a value, but then uses the matched command's boolean --color option to find the completion handler. It therefore returns nothing.

commander:       root.json
Active command behavior: prod [env for serve subcommand]
tab behavior:   nothing

its like a mixture of both lookup policies rather than a complete implementation of either.

I should revise this PR and the solution, I'd be happy hearing any feedback/suggestions from you.

[shadowspawn]

Thanks for reply and explanation! I'll try and take a bit of a dive into the bombshell parsing, and experiment, so I can make more informed comments.

[shadowspawn]

Slightly different reproduction, again confirming old implementation does not match Commander default masking anyway. This is just testing which completion handler and not arity. Using shipping @bomb.sh/tab@0.0.22.

const program = new Command('my-masking');
program
  .addOption(new Option('-c, --gc <number>', 'global choices').choices(['top', 'global']))

// Define commands
program.command('serve')
  .description('Start the server')
  .addOption(new Option('-c, --lc <number>', 'local choices').choices(['leaf', 'local']))
  .action((options) => {
    console.log('Starting server...');
  });

% my-masking complete -- -c ''
top	
global	
:4
% my-masking complete -- serve -c ''
leaf	
local	
:4
% my-masking serve -c leaf          
error: option '-c, --gc <number>' argument 'leaf' is invalid. Allowed choices are top, global.

[shadowspawn]

I did an end-to-end test with Commander to somewhat match reproduction in #153. Turned on Commander .enablePositionalOptions() to match masking expectations and used @bomb.sh/tab from fix/position-aware-parsing.

const program = new Command('repro-153');
program
  .enablePositionalOptions()
  .addOption(new Option('-H', 'global help'))

program
  .command('serve')
  .description('Serve the app')
  .addOption(new Option('-H, --host <hostname>', 'Host name').choices(['localhost', '']))
  .addArgument(new Argument('<env>', 'Environment').choices(['prod', 'staging']))
  .action((env, options) => {
    console.log(`Starting server with host ${options.host} and env ${env}`);
  });

% node repro-153.mjs complete -- -H ''
:4
% node repro-153.mjs complete -- serve -H ''
localhost	
	
:4
% node repro-153.mjs complete -- serve ''
prod	
staging	
:4
% node repro-153.mjs serve -H localhost prod
Starting server with host localhost and env prod

[shadowspawn]

Surveying runtimes looking at three questions:

• are options on root command global? (So recognised after subcommand.)
• are root and subcommand option with same short independent of each other. Can use root option before subcommand, and can use winning option after subcommand.
• if independent, which one consumes the clashing option in the subcommand context (after subcommand in args)

parser	root option global	clash independent	clash winner	configuration
cac	yes	no
citty	root options not supported
commander	yes	yes	root	.enablePositionaOptions(false)
commander	no	yes	subcommand	.enablePositionaOptions(true)
gunshi	root options not supported			Root option values not usable by author? Has ctx.addGlobalOption() which is option on every leaf, not root as such.
yargs	yes	no
proposed tab	yes	yes	subcommand

root option global means can have an option on the root command and use it after the subcommand name (in absence of a clash).

clash independent means can have a root option and subcommand option with different types and same short option, and can use the root option before the subcommand, and can use the clash winner after the subcommand. Types independent, values independent.

(Apologies in advance for any parsers I have misunderstood! I wrote example programs for each and consulted their documentation for how to configure the program.)

[shadowspawn]

If the option is not on matched command, I think you need to fallback and check the root command too?

Or more generally if deep command hierarchies are supported, need too check against commands from the leaf to the root (assuming nearest command wins).

[shadowspawn]

High level summary of my various comments:

• clashing root and leaf options are rare
• matching against leaf option for clash is reasonable
• Commander has various configurations that are not directly supported, but they don't need to be considered in this PR

I started going carefully through the PR and found one potential parsing problem. I stopped at that point partly because taking a break, partly because code might change in response.