Skip to content

fix: root cmd completions#22

Merged
Aslemammad merged 2 commits intomainfrom
fix/root-cmd-completion
Apr 28, 2025
Merged

fix: root cmd completions#22
Aslemammad merged 2 commits intomainfrom
fix/root-cmd-completion

Conversation

@AmirSa12
Copy link
Member

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Apr 12, 2025

⚠️ No Changeset found

Latest commit: 628b546

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Apr 12, 2025

Open in StackBlitz

npm i https://pkg.pr.new/bombshell-dev/tab@22

commit: 628b546

@AmirSa12 AmirSa12 requested a review from Aslemammad April 12, 2025 07:55
@Aslemammad
Copy link
Collaborator

i do not understand what this PR does! could you explain a bit?

@AmirSa12
Copy link
Member Author

AmirSa12 commented Apr 12, 2025

i do not understand what this PR does! could you explain a bit?

Hey!
Root command completions in zsh were not working as expected.
If you test, 'vite TAB' is not auto completing the subCommands of vite.
This PR fixes that issue.

src/index.ts Outdated
toComplete: string
) {
const commandParts = [...previousArgs];
const commandParts = [...previousArgs].filter(arg => arg !== '');
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can just do .filter(Boolean)

src/index.ts Outdated
let match = true;

// For root completion, just match the first part of each command
if (isRootCompletion) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think this is necessary or it should be that explicit! The while loop below that handles subcommands should already handle this.

@Aslemammad
Copy link
Collaborator

And I wonder why we did not caught this already in the tests, so there should be tests for this.

@Aslemammad Aslemammad requested review from 43081j and dreyfus92 April 12, 2025 11:34
@AmirSa12 AmirSa12 force-pushed the fix/root-cmd-completion branch from f8e021f to 628b546 Compare April 28, 2025 20:27
toComplete: string
) {
const commandParts = [...previousArgs];
const commandParts = [...previousArgs].filter(Boolean);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes the vite autocompletion issue because when you type vite , the shell might pass something like "vite", "" as the arguments array to the completion function (where the empty string represents the position where TAB was pressed) without this, those empty strings would be treated as actual command parts causing the matching logic to fail when comparing against available commands since no match would be found, no subcommands would be shown here! this cleans up the arguments array to only contain actual command parts like "vite" to make the matching logic to work correctly

Copy link
Collaborator

Choose a reason for hiding this comment

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

makes total sense!

@Aslemammad Aslemammad merged commit d6372ae into main Apr 28, 2025
9 checks passed
@Aslemammad Aslemammad deleted the fix/root-cmd-completion branch April 28, 2025 21:02
AmirSa12 added a commit that referenced this pull request Apr 29, 2025
* fix root cmd completions

* update
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.

2 participants