Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: parser-configuration should work well with generated completion script #2332

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AgentEnder
Copy link

There is an incompatibility between the parser configuration options and the completion feature within yargs. Setting strip-dashed: true in the parser configuration causes the completion feature to fail.

This is because the completion feature relies on passing '--get-yargs-completions' verbatim to yargs. Within the Completion class, the completionKey is explicitly set to get-yargs-completions. Within the YargsInstance, we explicitly check the following condition: completionKey in argv, in this location: main/lib/yargs-factory.ts#L2054

Since the argv has been parsed already at this point, the argument is now 'getYargsCompletions' instead of 'get-yargs-completions'. This causes the completion feature to never be called.

Fixes: #2330

@AgentEnder AgentEnder changed the title chore: add failing test for parser configuration and completion fix: parser-configuration should work well with generated completion script May 9, 2023
@shadowspawn
Copy link
Member

Long story. This is not quite right, but works in simple cases. I think there are less code changes required.

The strip-dashed spelling appearing in the "aliases" is a bit of cheat, it only appears in the parse results and is not recognised on the command-line and does not match what is on the command-line.

I added some trace statements to double-check my expectations. (I have a -d, --debug boolean option in my test program.)

At this original line:

      const requestCompletions = this.#completion!.completionKey in argv;

argv is the parse result from calling yargs-parser internally, so with default options:

$ node index.js --get-yargs-completions --debug
{
  argv: {
    _: [],
    'get-yargs-completions': true,
    getYargsCompletions: true,
    debug: true,
    d: true,
    '$0': 'index.js'
  }
}

To cope with the author changing the parser configuration, we need to be flexible about what the property got called in the parse results.

Later at this point:

        const completionArgs = args.slice(
           args.indexOf(`--${this.#completion!.completionKey}`) + 1
         );

args is the original command-line args like:

$ node index.js --get-yargs-completions --debug
{ args: [ '--get-yargs-completions', '--debug' ] }

Here, we still want to check for the original unaltered this.#completion!.completionKey no matter what "alias" we found in argv. With 'strip-dashed': true the new code looks for the stripped name and fails to remove the dashed argument.

So requestCompletions should stay just a boolean, and only that initial calculation changes. I think you can just change .find() to .some() to match the semantics. We don't care which variation was found.

Copy link
Member

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

See long comment! I think this is close, but doing too much.

@shadowspawn
Copy link
Member

This is because the completion feature relies on passing '--get-yargs-completions' verbatim to yargs.

This assumption is ok. The cause of the bug you observed is that the option name got changed in the parse results by the parser configuration settings, and fooled yargs.

@AgentEnder AgentEnder force-pushed the fix/strip-dashed-completion branch from e762ac2 to 095fdd8 Compare May 10, 2023 12:32
Copy link
Member

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

Thanks @AgentEnder

@shadowspawn
Copy link
Member

Good description with the PR, and added a test covering multiple configurations. Nice work.

@AgentEnder
Copy link
Author

Thanks for the help @shadowspawn!

Once it is merged, do you know when the next release would be? I've admittedly not paid too much attention to the yargs release schedule in the past.

@shadowspawn
Copy link
Member

I am guessing next release 1-2 months after last release (which was 2023-04-27).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: Incompatibility between parser configuration and completion
2 participants