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

feat(completion): print completion code to stdout #7597

Merged
merged 23 commits into from Feb 6, 2024

Conversation

KSXGitHub
Copy link
Contributor

@KSXGitHub KSXGitHub commented Jan 30, 2024

Resolves #3083

Unlike install-completion which would write files to the filesystem for you, completion only prints the code for completion to stdout. It is the responsibility of the user to convert the code in the stdout to a completion file and source them from their login shell.

Usage Example: Create completion script for ZSH and source them from .zshrc

pnpm completion zsh > ~/pnpm-completion.zsh
echo 'source "$HOME/pnpm-completion.zsh"' >> ~/.zshrc

@zkochan
Copy link
Member

zkochan commented Jan 30, 2024

The referenced issue mentions that helm and kubectl use the completion command. So, let's also name it pnpm completion.

@zkochan
Copy link
Member

zkochan commented Jan 30, 2024

actually, that would conflict with the completion command that is called by the tabtab script. I wonder if we should rename that command too. We can do these breaking changes as this will be released in pnpm v9.

@KSXGitHub
Copy link
Contributor Author

The referenced issue mentions that helm and kubectl use the completion command. So, let's also name it pnpm completion.

I suspect pnpm completion is already used. How else would the completion scripts be able to work if not through pnpm completion?

@KSXGitHub
Copy link
Contributor Author

actually, that would conflict with the completion command that is called by the tabtab script. I wonder if we should rename that command too. We can do these breaking changes as this will be released in pnpm v9.

I'm fine either way. generate-completion is symmetrical to the other commands install-completion and uninstall-completion. Besides, this command is only called once per user per setup.

@zkochan
Copy link
Member

zkochan commented Jan 30, 2024

npm has a completion command for doing exactly the same thing. And for getting the list of completions it uses npm completion --.

But it is also not super important, so I don't know.

@KSXGitHub
Copy link
Contributor Author

@zkochan I guess we can redirect pnpm completion -- $SHELL to pnpm generate-completion $SHELL for convenience. By "redirect", I mean child_process.spawnSync.

@KSXGitHub
Copy link
Contributor Author

But should we though? Overloading functionalities doesn't look maintainable in the long run.

@KSXGitHub
Copy link
Contributor Author

Besides, I think pnpm generate-completion $SHELL is clearer and more explicit than pnpm completion -- $SHELL. I do not know how npm users know npm completion -- $SHELL is the way to get completion code, but this behavior doesn't look intuitive to me. And if the user forgets to provide $SHELL argument for pnpm generate-completion, a helpful error message would appear and tell the user what they did wrong, instead of an empty output like the case of pnpm completion.

@zkochan
Copy link
Member

zkochan commented Jan 31, 2024

No, npm uses npm completion $SHELL to generate completion code (like helm and kubectl). npm completion -- is called by the completion script to generate the list of completions.

@KSXGitHub
Copy link
Contributor Author

npm completion -- is called by the completion script to generate the list of completions.

So this would still requires changes on tabtab codebase after all. The tabtab codebase is kind of a mess right now, tests failing left and right.

@zkochan
Copy link
Member

zkochan commented Jan 31, 2024

I guess, the command that tabtab executes can be renamed to something else, like pnpm list-completions for instance.

* switch to provided type declarations
* fix typings
* update tests
* update bundle scripts
Copy link
Contributor Author

@KSXGitHub KSXGitHub left a comment

Choose a reason for hiding this comment

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

@zkochan I require your inputs.

pnpm/src/cmd/complete.ts Outdated Show resolved Hide resolved
pnpm/src/cmd/complete.ts Outdated Show resolved Hide resolved
@KSXGitHub KSXGitHub changed the title feat(completion): generate-completion feat(completion): print completion code to stdout Feb 1, 2024
@KSXGitHub
Copy link
Contributor Author

I have renamed generate-completion to just completion.

pnpm/src/cmd/completion.ts Show resolved Hide resolved
pnpm/src/pnpm.ts Outdated Show resolved Hide resolved
@KSXGitHub KSXGitHub force-pushed the print-completions-instead-modification-3083 branch 2 times, most recently from 8f03869 to 6ae45a0 Compare February 4, 2024 18:18
@KSXGitHub KSXGitHub marked this pull request as draft February 4, 2024 18:21
@KSXGitHub KSXGitHub force-pushed the print-completions-instead-modification-3083 branch 7 times, most recently from 64406ed to 061bd0b Compare February 5, 2024 11:47
@KSXGitHub KSXGitHub force-pushed the print-completions-instead-modification-3083 branch from 061bd0b to 738c090 Compare February 5, 2024 18:21
Just `pnpm completion` is enough
@KSXGitHub
Copy link
Contributor Author

The commands install-completion and uninstall-completion have been removed (ed658a6).

@KSXGitHub KSXGitHub marked this pull request as ready for review February 5, 2024 18:28
Copy link
Member

@zkochan zkochan left a comment

Choose a reason for hiding this comment

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

I don't think 2 plugins packages are needed. All the completion related commands can be in one package.

@KSXGitHub
Copy link
Contributor Author

I have merged plugin-commands-completion-server into plugin-commands-completion.

Copy link
Member

@zkochan zkochan left a comment

Choose a reason for hiding this comment

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

The changesets don't describe all the changes.

pnpm/src/pnpm.ts Outdated
@@ -15,8 +15,13 @@ const argv = process.argv.slice(2)
break
}
case 'install-completion': {
Copy link
Member

Choose a reason for hiding this comment

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

you said you'll remove install-completion and uninstall-completion from the commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I converted them to plugins then forgot to remove them here. When I remove them, I completely forgot to remove the switch block.

@zkochan zkochan merged commit 004addf into main Feb 6, 2024
13 of 14 checks passed
@zkochan zkochan deleted the print-completions-instead-modification-3083 branch February 6, 2024 22:18
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.

Should not modify user shell dotfiles
2 participants