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

Reworked gen-bash-completion into a more generic completion command #1967

Merged
merged 5 commits into from
Sep 24, 2018

Conversation

Tirke
Copy link
Contributor

@Tirke Tirke commented Sep 21, 2018

Why ?

I'm using Zsh (and I'm not the only one 🤣). Pulumi having Zsh completions is great. I will also add completions to the Homebrew Formula when this is merged.

Why not use Cobra GenZshCompletion

It's currently not good enough. Maybe it will be when spf13/cobra#646 is done.

Implementation

I did the same thing kubectl does for Zsh completion. Meaning using the bash completion generated by Cobra and adapting it to a zsh format. The resulting zsh completion file is not perfect (compared to one's where you have a short command description in the output) but it's good enough I think.

I also changed the file output to a stdout output. I think it's better than outputting to a file and it will make adding completions in Homebrew straightforward. I don't know if the previous gen-bash-completion is used in any Pulumi project so this may break things.

@Tirke
Copy link
Contributor Author

Tirke commented Sep 21, 2018

I'm sorry I don't really get why the build is failing and I have a hard time making Gometalinter work locally.

@ellismg
Copy link
Contributor

ellismg commented Sep 22, 2018

I'm sorry I don't really get why the build is failing and I have a hard time making Gometalinter work locally.

I pushed a commit to your branch that addresses the linter issues.

@ellismg
Copy link
Contributor

ellismg commented Sep 22, 2018

I don't know if the previous gen-bash-completion is used in any Pulumi project so this may break things.

We don't appear to use it ourselves, so I think it's safe to make this change.

@Tirke
Copy link
Contributor Author

Tirke commented Sep 22, 2018

Thanks for the lint fix, I thought I didn't need to handle errors on print functions but it makes sense.

Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

Thanks Tirke!

I believe the only place this is used currently is in the docs at
https://github.com/pulumi/docs/blob/f426b531e461c62ec7c55ca884dcc6b133aca9bb/reference/commands.md

If you want to send a PR there for the changes to adapt to this that would be great!

// It is hidden by default since it's not commonly used outside of our own build processes.
func newCompletionCmd(root *cobra.Command) *cobra.Command {
return &cobra.Command{
Use: "completion <SHELL>",
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should keep this command name closer to the original - gen-completion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fine with me

@lukehoban lukehoban merged commit cb9cb7d into pulumi:master Sep 24, 2018
@Tirke Tirke deleted the add-zsh-completion branch September 25, 2018 14:53
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

3 participants