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: subCommands find problem #179

Merged
merged 4 commits into from
Mar 6, 2024
Merged

fix: subCommands find problem #179

merged 4 commits into from
Mar 6, 2024

Conversation

life2015
Copy link
Contributor

@life2015 life2015 commented Mar 2, 2024

First of all, thanks for this awesome project, which has given me a lot of inspiration in the areas of terminal experience and smart completion. I have recently been trying to integrate it with Xterm.js. However, I encountered some issues while using it and reading the code.

What is the problem ?

When I type npm run, the terminal autocomplete returns the incorrect result "--registry", instead of the scripts in package.json or other run parameters.

image

This behavior is somewhat strange, so I delved into the source code to look for the issue.

After spending some time debugging, the problem was finally located in the genSubCommand method of the runtime.ts file.

image

Here, we can see that when typing npm run, function genSubCommand attempting to find s.name === 'run' within ·parentCommand.subcommands· is unsuccessful, returning subcommandIdx is -1.

However, the judgment logic in the code is SubCommand == null, which obviously does not apply when subCommand is -1. As a result, it proceeds to parentCommand.subcommands.at(-1), effectively retrieving the last item in the subcommands list (the 69th item). Therefore, the output becomes --registry instead of the correct result.

image

How to Fix

image

By inspecting the contents of parentCommand.subcommands through the Debugger, it can be observed that the name of a subCommand might be an array of strings. And the type of subCommand also prove this.

image

Therefore, we need to fix the find matching logic for subCommands to handle both arrays and strings during the match.

  const subcommandIdx = parentCommand.subcommands?.findIndex((s) => {
    // subCommand.name can be a string or an array of strings
    if (Array.isArray(s.name)) {
      // if it's an array, we check if the command is included in the array
      return s.name.includes(command);
    } else {
      return s.name === command;
    }
  });

Additionally, we also need to optimize the verification logic before and after the find match to handle exceptional cases.

  if (!parentCommand.subcommands || parentCommand.subcommands.length === 0) return;
  // ... find logic 
  if (subcommandIdx === -1) return;

After fix

After fix, npm run looks great !
image

@life2015
Copy link
Contributor Author

life2015 commented Mar 4, 2024

@cpendery

Copy link
Member

@cpendery cpendery left a comment

Choose a reason for hiding this comment

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

Amazing fix, thank you for the detailed write up and PR!! Just a few minor nits and it looks good to go!

src/runtime/runtime.ts Outdated Show resolved Hide resolved
src/runtime/runtime.ts Outdated Show resolved Hide resolved
life2015 and others added 2 commits March 5, 2024 13:50
Co-authored-by: Chapman Pendery <[email protected]>
Co-authored-by: Chapman Pendery <[email protected]>
@life2015
Copy link
Contributor Author

life2015 commented Mar 5, 2024

Amazing fix, thank you for the detailed write up and PR!! Just a few minor nits and it looks good to go!

Great patch!
Just applied these changes!

cpendery
cpendery previously approved these changes Mar 6, 2024
Signed-off-by: Chapman Pendery <[email protected]>
@cpendery cpendery merged commit bca4498 into microsoft:main Mar 6, 2024
3 checks passed
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.

None yet

2 participants